Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preparing convict 6 #350

Closed
madarche opened this issue Dec 28, 2019 · 17 comments
Closed

Preparing convict 6 #350

madarche opened this issue Dec 28, 2019 · 17 comments

Comments

@madarche
Copy link
Collaborator

@A-312, since we are preparing a new major version, are there any breaking changes that you would like to be included in 6.0.0?

@A-312

This comment has been minimized.

@A-312

This comment has been minimized.

@A-312

This comment has been minimized.

@A-312
Copy link
Contributor

A-312 commented Jan 1, 2020

Can you rebase mozilla:feat-multi-packages-split into master ? Because this commit is missing : d263763

@A-312

This comment has been minimized.

@A-312 A-312 mentioned this issue Jan 5, 2020
14 tasks
@A-312
Copy link
Contributor

A-312 commented Jan 5, 2020

I did the work of merging & resolving conflict in my PR#353 to save some time and let you merge the 6.0.0 with only one click :) (You just need to write the CHANGELOG.md and fix my english :') )

@A-312
Copy link
Contributor

A-312 commented Jan 10, 2020

I will make another PR (for 6.0.0), to support default like property name in config.

Today or tomorrow, I will improve default usage in parsing of schema and in validate (to be more user friendly) (#337), you can merge my PR but just wait my last change.

@A-312
Copy link
Contributor

A-312 commented Jan 11, 2020

I would like to take advantage of these break changes to rename some function. After reading the new doc of #314: https://github.com/mozilla/node-convict/pull/314/files#diff-24ac9951b3e0cda0a70e9b9877953990R306 I think merge is more appropriate function name that load or loadFile. Because when we use load we merge configs depending of getters priority.

@madarche Should we rename/merge .load(object) and .loadFile(string | array) into a new function .merge(object | string | array) ?

We can mark load and loadFile to deprecated and remove it for convict v7 or v8. Or remove this 2 functions now to force people to read the changelog/readme of 6.0.0.

@A-312
Copy link
Contributor

A-312 commented Jan 20, 2020

Object-path was updated this morning but not yet published on npm.

@madarche
Copy link
Collaborator Author

@A-312 I'm working on master to update the dependencies, and I'm new to lerna: I've just read about but not actually used it before.

On my local repository I've changed the 4 package.json files and I now want to update the corresponding package-lock.json files.

From what I've read I think the following command should do what I want:

$ npx lerna exec -- npm i

I've also tried the following command, but I'm really less sure this is what I need to just update the corresponding package-lock.json files:

$ npx lerna bootstrap

But both commands produce the same kind of warnings:

lerna WARN ECYCLE Dependency cycles detected, you should fix these!
lerna WARN ECYCLE convict-format-with-moment -> convict -> convict-format-with-moment
lerna WARN ECYCLE convict-format-with-validator -> convict -> convict-format-with-moment -> convict
lerna WARN ECYCLE convict-format-with-moment -> convict -> convict-format-with-validator -> convict
lerna WARN ECYCLE convict-format-with-validator -> convict -> convict-format-with-validator
lerna WARN ECYCLE convict -> convict-format-with-moment -> convict

and finish with a 1 error code.

What commands should I use? Or is it a real problem?

Beside, could you recommend me a project which is using lerna and has documented its commands for contributing? A beginner's guide?

@A-312 could you help me please?

@A-312
Copy link
Contributor

A-312 commented Mar 19, 2020

@madarche It the version script tag in your package.json that cause your problem.

I forget to edit. I mean this one : https://github.com/mozilla/node-convict/blob/master/assert_changelog_ready

(I published my fork https://www.npmjs.com/package/blueconfig because I had to use it for my project, you can use it like example)

@madarche
Copy link
Collaborator Author

Thank you @A-312, I'll use https://www.npmjs.com/package/blueconfig as a model.

I'll report here soon.

@madarche
Copy link
Collaborator Author

@A-312 the solution to fix the Dependency cycles detected warnings was to remove all references to convict-format-with-moment and convict-format-with-validator from convict. This has been fixed with 5a16dd1. The main change was in convict's package.json.

I've checked-out the code of blueconfig and the Dependency cycles detected warnings are there too:

node-blueconfig (master)$ npx lerna exec -- npm i
lerna notice cli v3.20.2
lerna info Executing command in 3 packages: "npm i"
lerna WARN ECYCLE Dependency cycles detected, you should fix these!
lerna WARN ECYCLE blueconfig-format-with-moment -> blueconfig -> blueconfig-format-with-moment
lerna WARN ECYCLE blueconfig-format-with-validator -> (nested cycle: blueconfig-format-with-moment -> blueconfig -> blueconfig-format-with-moment) -> blueconfig-format-with-validator
lerna WARN ECYCLE (nested cycle: blueconfig-format-with-validator -> (nested cycle: blueconfig-format-with-moment -> blueconfig -> blueconfig-format-with-moment) -> blueconfig-format-with-validator) -> (nested cycle: blueconfig-format-with-validator -> (nested cycle: blueconfig-format-with-moment -> blueconfig -> blueconfig-format-with-moment) -> blueconfig-format-with-validator)

@A-312
Copy link
Contributor

A-312 commented Apr 11, 2020

I think npx lerna exec -- npm i is not a good command, maybe you should do: npm bootstrap ?

@A-312
Copy link
Contributor

A-312 commented Apr 11, 2020

In blueconfig, I think I will remove devDependencies in packages/*/package.json

@madarche
Copy link
Collaborator Author

@A-312:

  • OK for lerna bootstrap! Thanks!
  • Acknowledged about intention to remove devDependencies in packages/*/package.json. I'm investigating different options in the same area, to see if it's possible/desirable to have test coverage globally and/or separately for each package.

@madarche
Copy link
Collaborator Author

madarche commented May 2, 2020

convict@6.0.0 is now ready for publishing. All the points related to the use of lerna have been dealt with #368.

@madarche madarche closed this as completed May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants