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

Stop scanning node_modules directories in PackageSource#_findSources. #9825

Merged
merged 1 commit into from Apr 18, 2018

Conversation

Projects
None yet
1 participant
@benjamn
Member

benjamn commented Apr 18, 2018

This functionality was originally intended to allow importing compiled-to-JS modules from node_modules, by precompiling any modules found in top-level npm packages, as long as a Meteor compiler plugin was registered for the module's file extension.

As discussed in #9800, this extra compilation burden can be non-trivial, especially if you happen to install an npm package such as less, which contains hundreds of .less files in the node_modules/less/test/ directory.

More generally, this functionality was an early attempt to enable selective compilation of node_modules directories, but it was not a good solution to that problem, because in almost all cases the extra compilation was unwanted.

Meteor 1.7 (formerly known as 1.6.2) will give full control over selective compilation of node_modules back to the application developer (#9771), which should afford a much better solution to this problem. If you've installed some .less or .scss or .ts files from npm into your node_modules directory, just create a symlink to the package directory within your application, and those modules will be compiled and become importable by other JS modules, as if they were part of the application.

Stop scanning node_modules directories in PackageSource#_findSources.
This functionality was originally intended to allow importing
compiled-to-JS modules from `node_modules`, by precompiling any modules
found in top-level npm packages, as long as a Meteor compiler plugin was
registered for the module's file extension.

As discussed in #9800, this extra compilation burden can be non-trivial,
especially if you happen to install an npm package such as `less`, which
contains hundreds of `.less` files in the `node_modules/less/test/`
directory.

More generally, this functionality was an early attempt to enable
selective compilation of `node_modules` directories, but it was not a good
solution to that problem, because in almost all cases the extra
compilation was unwanted.

Meteor 1.7 (formerly known as 1.6.2) will give full control over selective
compilation of `node_modules` back to the application developer (#9771),
which should afford a much better solution to this problem. If you've
installed some `.less` or `.scss` or `.ts` files from npm into your
`node_modules` directory, just create a symlink to the package directory
within your application, and those modules will be compiled and become
importable by other JS modules, as if they were part of the application.

@benjamn benjamn added this to the Release 1.7 milestone Apr 18, 2018

@benjamn benjamn merged commit c5302bb into release-1.6.2 Apr 18, 2018

20 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

benjamn added a commit that referenced this pull request May 29, 2018

Revert "Stop scanning node_modules directories in PackageSource#_find…
…Sources. (#9825)"

This reverts commit c5302bb.

Based on conversation with @KoenLav, it seems this optimization was more
of a breaking change than anticipated, and the workaround of creating a
symbolic link to induce compilation is not just annoying on Windows but
also does not satisfy the goal of being able to import from node_modules
(as before) rather than importing via the symbolic link.

We will need to revisit this pitfall in Meteor 1.7.1, but it's important
to unbreak it ASAP in Meteor 1.7.0.1 (since 1.7 is unfortunately final).

#9826 (comment)
mozfet/meteor-autoform-materialize#43

For apps that are accidentally compiling too many LESS or SCSS files, the
recommended workaround (for now) is to add the offending directories
and/or files to a .meteorignore file.

benjamn added a commit that referenced this pull request May 29, 2018

@benjamn benjamn referenced this pull request May 29, 2018

Merged

Release 1.7.0.1 #9917

benjamn added a commit that referenced this pull request Jun 7, 2018

Avoid compiling lazy .less files unless they are imported.
During the Meteor 1.7 beta testing process, I noticed that npm packages
installed in `node_modules` that have a lot of `.less` files in them (such
as the `less` npm package or `antd`) can slow down rebuilds significantly,
because every `.less` file gets compiled, even if it is never used.

With #9825, I attempted to solve this problem by completely removing the
logic for scanning `node_modules` for files to process with compiler
plugins, but that turned out to be a breaking change for existing apps
that relied on that behavior, so we reverted that optimization in Meteor
1.7.0.1: #9917

Of course, this means the performance problem still occurs in Meteor
1.7.0.1, as demonstrated by #9957. After investigating the problem a bit
further, I discovered that the `LessCompiler#isRoot` method was simply
ignoring whether a `.less` file comes from `node_modules` or not.

More specifically, Meteor sets `fileOptions.lazy = true` by default for
any file found in `node_modules`
[here](https://github.com/meteor/meteor/blob/0a336175c4a47cd264060891b5f66893b945afe4/tools/isobuild/package-source.js#L1020),
so `LessCompiler#isRoot` should return `false` from `isRoot` when
`fileOptions.lazy` is truthy.

Thanks to this much more precise solution, `.less` files in `node_modules`
can still be imported by other `.less` files; they just won't be eagerly
compiled by the `less` package.

Fixes #9957.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment