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

Add option to include additional packages #8769

Merged
merged 12 commits into from Jun 21, 2017

Conversation

@mpowaga
Copy link
Contributor

@mpowaga mpowaga commented Jun 6, 2017

This is rework of #8735 as discussed in #8739.

Example usage:

meteor run --extra-packages "bundle-visualizer, jquery@1.11.10"

Things to consider:

  • should be renamed to --with-packages or other name?
  • should be added to meteor build?
  • in case when the same package is already specified in .meteor/packages but with different version constraint should --extra-packages override it?
Copy link
Member

@hwillson hwillson left a comment

Hi @mpowaga - thanks for re-submitting this PR! I've added a few small code comments inline.

One thing I think is missing though are tests for this. I know creating selftest's for the build tool can be a bit of pain, but in this case it shouldn't be that bad. You can create a test app in tools/tests/apps, then fire this test app up with the --extra-packages option set with some package (take a look at the tests in tools/tests/command-line.js to get an idea of how to do this). Once the app is loaded, you could then just make a call that requires the package to be loaded, and verify the call succeeded.

Everything else looks great - thanks again!

@@ -289,10 +290,16 @@ function doRunCommand(options) {
const { parsedServerUrl, parsedMobileServerUrl } =
parseServerOptionsForRunCommand(options, runTargets);

var includePackages = [];
if (options['extra-packages']) {
includePackages = options['extra-packages'].split(',');

This comment has been minimized.

@hwillson

hwillson Jun 7, 2017
Member

You might want to strip blank spaces after your comma split, to handle your example of "package1, package2" properly. I know you're trimming later on down the chain in your code changes, but it might be useful to just trim right alongside the split. So something like:

includePackages = options['extra-packages'].split(',').map(item => item.trim());

This comment has been minimized.

@abernix

abernix Jun 7, 2017
Member

Agree with this, but I think I'd have a strong preference to:

includePackages = options['extra-packages'].trim().split(/\s*,\s*/);

...which I believe would be more performant, though the chances of someone passing too many packages here is slim.

EDITED FOR CORRECTNESS! ;)

@@ -86,6 +86,7 @@ Options:
downgraded to versions that are potentially incompatible with
the current versions, if required to satisfy all package
version constraints.
--extra-packages Run with additional packages (comma separated)

This comment has been minimized.

@hwillson

hwillson Jun 7, 2017
Member

Maybe add an example here (similar to the --mobile-server option help):

--extra-packages Run with additional packages (comma separated, for example: --extra-packages "package-name1, package-name2@1.2.3")
Copy link
Member

@benjamn benjamn left a comment

Besides @hwillson's requested changes, we agreed --extra-packages is a fine name, and there is no need to support this (yet) for meteor build or meteor deploy (since that's kind of what the prodOnly flag is for).

However, we would like this option to work with meteor test ... and meteor test-packages ....

To your final question, yes, I think --extra-packages should be able to override version constraints from .meteor/packages.

Thanks for working on this!

@mpowaga
Copy link
Contributor Author

@mpowaga mpowaga commented Jun 8, 2017

Added this option to test and test-packages commands as well.

Extra packages will now override constraints specified in .meteor/packages. This added a little bit of complexity as I had to add skipOnRead property to mark constraints that were overridden. There's also a danger that extra packages will be written to .meteor/packages. To prevent this I added another property skipOnWrite. Perhaps this should be covered by tests.

I also created selftest's. Though I'm not sure if they are in the right place since they do a little more than just checking "argument parsing".

/cc @hwillson @benjamn

@@ -985,7 +1013,7 @@ _.extend(exports.ProjectConstraintsFile.prototype, {
eachConstraint: function (iterator) {
var self = this;
_.each(self._constraintLines, function (lineRecord) {
if (lineRecord.constraint)
if (! lineRecord.skipOnRead && lineRecord.constraint)

This comment has been minimized.

@zimme

zimme Jun 9, 2017
Contributor

If eachConstraint ignores extra-packages maybe this needs to happen in getConstraint and the other functions too? So should this really be skipping in this case?

This comment has been minimized.

@mpowaga

mpowaga Jun 9, 2017
Author Contributor

eachConstraint iterates through self._constraintLines which may contain duplicate packages. This is because it needs to preserve all constraints from .meteor/packages (so this file can be later reproduced by _write method) as well as self._includePackages.

On the contrary, getConstraint reads from self._constraintMap which contains unique packages.

Maybe there should be separate property for extra constraints instead of pushing them to self._constraintLines? For example self._extraConstraintLines. But then eachConstraint would need iterate through self._extraConstraintLines and self._constraintLines and find duplicates along the way.

This comment has been minimized.

@zimme

zimme Jun 9, 2017
Contributor

eachConstraint iterates through self._constraintLines which may contain duplicate packages. This is because it needs to preserve all constraints from .meteor/packages (so this file can be later reproduced by _write method) as well as self._includePackages.

On the contrary, getConstraint reads from self._constraintMap which contains unique packages.

Sounds reasonable.

Maybe there should be separate property for extra constraints instead of pushing them to self._constraintLines? For example self._extraConstraintLines. But then eachConstraint would need iterate through self._extraConstraintLines and self._constraintLines and find duplicates along the way.

Feels like that might be overkill but I'm not too familiar with this part of the code.

This comment has been minimized.

@mpowaga

mpowaga Jun 9, 2017
Author Contributor

Feels like that might be overkill but I'm not too familiar with this part of the code.

Yes, that would be unnecessarily complex solution.

Another option would be to iterate over values of self._constraintMap. But I'm not sure if eachConstraint must iterate with correct order (first-in, first-out; as defined in .meteor/packages) and if so whether this would be still guaranteed with this approach.

This comment has been minimized.

@zimme

zimme Jun 9, 2017
Contributor

But I'm not sure if eachConstraint must iterate with correct order (first-in, first-out; as defined in .meteor/packages)

I'm pretty sure that ordering of the packages in .meteor/packages matters.

@zimme
Copy link
Contributor

@zimme zimme commented Jun 10, 2017

This PR only have changes that affect projectConstraintsFile and not packageMapFile which represents the .meteor/versions file. Will that be an issue?

I'm thinking about meteor/meteor-feature-requests#31, where we're waiting for this PR.
We need to search though all packages including indirect dependencies and we're thinking that looking at .meteor/versions is the best way to do so.

~~It would be nice if we can just look through packageMapFile and in doing so it will include the extra-packages as well, which means this PR would have to add the extra packages to packageMapFile to. This way we wouldn't need to look through includePackages as well.~~~

@zimme
Copy link
Contributor

@zimme zimme commented Jun 10, 2017

Related to my previous comment.

Turns out we might use projectContext.resolveConstraints() and packageMap.getInfo('<package-name>'). resolveConstraints() actually use eachConstraints and if extra-packages are ignored there we will have to look for the extra packages somewhere else.

This actually made be think that the constraint resolver will ignore extra packages if we ignore them in eachConstraints, will this have unwanted side-effects?

@mpowaga
Copy link
Contributor Author

@mpowaga mpowaga commented Jun 10, 2017

constraint resolver will ignore extra packages if we ignore them in eachConstraints

eachConstraint doesn't ignore extra packages. It only filters out overridden packages. So if you have jquery in .meteor/packages and run meteor with --extra-packages "jquery@1.11.0" it will return only one instance of jquery constraint, i.e. jquery@1.11.0.

@zimme
Copy link
Contributor

@zimme zimme commented Jun 10, 2017

Ah, I mixed up skipOnRead and skipOnWrite. My concerns have been put to rest.

@zimme
Copy link
Contributor

@zimme zimme commented Jun 11, 2017

Unless anyone can see a downside in doing the same with packageMapFile as you're doing with projectConstraintsFile I would really want to see that in this PR.

It's related to meteor/meteor-feature-requests#31.
If we do the same with packageMapFile as with projectConstraintsFile we can just use a find()/indexOf on packageMapFile.getCachedVersions().keys() to see if the package we're looking for is in the project or not, even if it's not a top-level dependency.

We thought about using packageMap but for that we need to run resolveConstraints() which is "expensive" and would add to the startup time.

@hwillson
Copy link
Member

@hwillson hwillson commented Jun 14, 2017

Thanks for your work on this @mpowaga - looks like all outstanding requests/suggestions have been addressed (and your self-test's look great!).

@zimme Based on your findings in meteor/meteor-feature-requests#31, do you have any remaining concerns about the changes in this PR?

@hwillson
Copy link
Member

@hwillson hwillson commented Jun 14, 2017

@mpowaga Actually, one small nitpick - could you wrap the help text example a little earlier, to keep it under 80 chars wide? Maybe just move everything after example: to a new indented line.

screenshot 2017-06-14 09 02 22

@zimme
Copy link
Contributor

@zimme zimme commented Jun 14, 2017

Right now this PR only adds the extra packages to projectConstraintsFile (.meteor/packages) with the filtering logic (skipOnRead/skipOnWrite) to not save the extra packages to the file and to not iterate over duplicate packages because a package was overridden via extra-packages.

In meteor/meteor-feature-requests#31 I'm looking through packageMapFile (.meteor/versions) for the package, as that includes all packages in the project including dependencies. So adding the extra packages to packageMapFile with the same filter logic would be nice because then I can just use packageMapFile.getInfo() to see if the package is available in the project as a direct or indirect dependency, and it would include the extra packages as well.

@zimme
Copy link
Contributor

@zimme zimme commented Jun 14, 2017

@mpowaga Actually, one small nitpick - could you wrap the help text example a little earlier, to keep it under 80 chars wide? Maybe just move everything after example: to a new indented line.

How about using package1, package2@1.2.3?

@mpowaga
Copy link
Contributor Author

@mpowaga mpowaga commented Jun 14, 2017

@hwillson Something like this:

  --extra-packages Run with additional packages (comma separated, for example:
                   --extra-packages "package-name1, package-name2@1.2.3")

?

@hwillson
Copy link
Member

@hwillson hwillson commented Jun 14, 2017

Yes, that formatting looks great - thanks!

@hwillson
Copy link
Member

@hwillson hwillson commented Jun 14, 2017

Thanks for the update @zimme - makes sense. Given that (I think) we're really close to merging this PR, maybe we should merge as is (to get the --extra-packages benefit sooner), then re-visit adding packageMapFile support afterwards. We have a triage call today and this PR will be discussed, so we'll see what the consensus is. Thanks again for the details!

@zimme
Copy link
Contributor

@zimme zimme commented Jun 14, 2017

I would have no problem adding the extended support for extra-packages in the branch I've created for meteor/meteor-feature-requests#31.

@hwillson hwillson added this to the Release 1.5.1 milestone Jun 14, 2017
@hwillson
Copy link
Member

@hwillson hwillson commented Jun 16, 2017

Hi all - I've added a History.md entry, so as soon as the tests re-run, we should be good to go here. LGTM!

@benjamn benjamn merged commit bc5db29 into meteor:devel Jun 21, 2017
3 checks passed
3 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
abernix added a commit that referenced this pull request Jul 25, 2017
The suggestion to `meteor add` and then `meteor remove` is no longer
relevant with the addition of the awesome new `--extra-packages`
option from @mpowaga in #8769.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.