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

Support meteor.nodeModules.recompile package.json configuration option. #10603

Merged

Conversation

@benjamn
Copy link
Member

benjamn commented Jul 1, 2019

In the spirit of meteor.mainModule and meteor.testModule (#9690), this PR introduces a new package.json-based configuration for selecting npm packages to recompile using the Meteor compiler plugins system. This configuration is intended to replace the unconditional compilation introduced by #10585.

Example:

{
  "name": "your-application",
  ...
  "meteor": {
    "mainModule": ...,
    "testModule": ...,
    "nodeModules": {
      "recompile": {
        "very-modern-package": ["client", "server"],
        "alternate-notation-for-client-and-server": true,
        "somewhat-modern-package": "legacy",
        "another-package": ["legacy", "server"]
      }
    }
  }
}

The keys of the meteor.nodeModules.recompile configuration object are npm package names, and the values specify for which bundles those packages should be recompiled using the Meteor compiler plugins system, as if the packages were part of the Meteor application.

For example, if an npm package uses const/let syntax or arrow functions, that's fine for modern and server code, but you would probably want to recompile the package when building the legacy bundle. To accomplish this, specify ”legacy" or ["legacy"] as the value of the package's property, similar to somewhat-modern-package above. These strings and arrays of strings have the same meaning as the second argument to api.addFiles(files, where) in a package.js file.

This configuration serves pretty much the same purpose as symlinking an application directory into node_modules, but without any symlinking: https://forums.meteor.com/t/litelement-import-litelement-html/45042/8?u=benjamn

The meteor.nodeModules.recompile configuration currently applies to the application node_modules directory only (not to Npm.depends dependencies in Meteor packages). Recompiled packages must be direct dependencies of the application.

@benjamn benjamn added this to the Release 1.8.2 milestone Jul 1, 2019
@benjamn benjamn self-assigned this Jul 1, 2019
Example:

  "meteor": {
    "mainModule": ...,
    "testModule": ...,
    "nodeModules": {
      "recompile": {
        "very-modern-package": ["client", "server"],
        "alternate-notation-for-client-and-server": true,
        "somewhat-modern-package": "legacy",
        "another-package": ["legacy", "server"]
      }
    }
  }

The keys of the meteor.nodeModules.recompile configuration object are npm
package names, and the values specify for which bundles those packages
should be recompiled using the Meteor compiler plugins system, as if the
packages were part of the Meteor application.

For example, if an npm package uses const/let syntax or arrow functions,
that's fine for modern and server code, but you would probably want to
recompile the package when building the legacy bundle. To accomplish this,
specify "legacy" or ["legacy"] as the value of the package's property,
similar to somewhat-modern-package above. These strings and arrays of
strings have the same meaning as the second argument to
api.addFiles(files, where) in a package.js file.

This configuration serves pretty much the same purpose as symlinking an
application directory into node_modules, but without any symlinking:
https://forums.meteor.com/t/litelement-import-litelement-html/45042/8?u=benjamn

The meteor.nodeModules.recompile configuration currently applies to the
application node_modules directory only (not to Npm.depends dependencies
in Meteor packages). Recompiled packages must be direct dependencies of
the application.
@benjamn benjamn force-pushed the meteor.nodeModules.recompile-package.json-config branch from 612d4a7 to f61ba60 Jul 1, 2019
benjamn added 14 commits Jun 28, 2019
In PR #10585, I tried enabling full compiler plugin processing for any
modules imported from `node_modules` that were not otherwise handled by
compiler plugins, but doing that for all packages was just too slow, not
to mention potentially dangerous for modules whose code cannot be safely
recompiled by Babel.

The primary reasons for wanting to recompile node_modules are

  1. to enable "native" ECMAScript `import`/`export` syntax (given that
     Node.js still does not fully support ESM syntax yet), and

  2. to compile standard syntax like `const`, `let`, classes, and arrow
     functions for legacy browsers.

The first goal can be achieved by compiling unanticipated `node_modules`
code with Reify, which is much faster than Babel, in part because Reify
can avoid doing any parsing when the source contains no `import` or
`export` identifiers. This compilation is also cached on disk, so its
impact should only be felt during cold builds after a `meteor reset`.

The second goal can be accomplished using the new `package.json`
configuration option `meteor.nodeModules.recompile`, which causes the
recompiled packages to be handled by the normal compiler plugins system,
so that the `ImportScanner`'s fallback compilation is not necessary.
Before this option was introduced, this recompilation behavior could be
achieved by symlinking application directories into `node_modules`, but
that always felt like an esoteric hack.
This ensures we wrap modules with a function to rename the `module`
identifier to something more reliable when the ImportScanner compiles
unanticipated modules imported from node_modules.
This partially reverts f0d39b8 by simply
including .js and .mjs modules in the server bundle, rather than
delegating to pure Node evaluation. In practice, whether or not the
package has a "module" entry point (or "type":"module") in its
package.json was not a perfect indicator of whether it should be compiled
with Reify and bundled, or left untouched and handled by Node.

Truly native modules (such as those with a .node file extension) should
always be handled by Node, so module.useNode() definitely still has a role
to play in the server bundle.
With appropriate meteor.nodeModules.recompile configuration, that is.
This partially reverts commit 4dfc741.

Some server packages, especially those that rely on __dirname or
__filename (e.g. puppeteer), simply cannot be included in the server
bundle, and must be evaluated natively by Node.

As long as we have a Module.prototype._compile hook to process natively
evaluated modules with Reify, module.useNode() can still benefit from ESM
import/export syntax.
This allows us to take advantage of the features.compileForShell option
when compiling code in the `meteor shell` REPL.
This saves us from having to install @babel/parser in the server bundle.

There is currently a bug in Reify that prevents it from loading this
parser by default, but we can work around that on the Meteor side for now.
We don't need to build a whole new dev bundle just for this upgrade, since
we've already worked around the bug that it fixes, but it will get picked
up the next time we build the dev bundle for Meteor 1.8.2.
@benjamn benjamn merged commit 2973e8a into release-1.8.2 Jul 2, 2019
17 of 19 checks passed
17 of 19 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
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/pr AppVeyor build succeeded
Details
benjamn added a commit that referenced this pull request Jul 3, 2019
Now that we have the meteor.nodeModules.recompile configuration (#10603),
we don't need to guess which packages might need to be recompiled.
@CaptainN

This comment has been minimized.

Copy link
Contributor

CaptainN commented Jul 18, 2019

Would it be possible to add an additional prop which would override the source entry point defined in "main" or "module".

I'm thinking about "material-ui" in particular. It ships ES6+ modules, in addition to pre-transpiled esm files, which is what it links in it's module field. But there's no obvious way to access that. With an override, we could do that ourselves, and possible gain some benefit in the modern bundle.

  "main": "./index.js",
  "module": "./esm/index.js",
  "name": "@material-ui/core",

That's what shows in "@material-ui/core"'s package.json file. If we could redefine that to use "module": "./es/index.js" that could be very useful. I'm not sure how many packages deploy assets such as this.

{
  "meteor": {
    "nodeModules": {
      "recompile": {
        "@material-ui/core": true
      },
      "alternaiveModule": { // or maybe "moduleEntries"
        "@material-ui/core": "./es/index.js"
      }
    }
  }
}
@trusktr

This comment has been minimized.

Copy link
Contributor

trusktr commented Aug 2, 2019

I feel like compile may be a better term than recompile, because not all node_modules code is necessarily compiled in the first place. Some people publish hand-written JS for common denominators of environments, or just modern code and then use their own build tools to consume the packages.

@trusktr

This comment has been minimized.

Copy link
Contributor

trusktr commented Aug 2, 2019

Would it be possible to add an additional prop which would override the source entry point defined in "main" or "module".

That would be a great feature!

Sometimes module might point to code with everything compiled except for import/export statements, but we may want to actually import relative to the src folder which some packages do publish. This would give us more control over the output by letting us compile original sources (when those are present).

It could get tricky though, as tsconfig files can vary from project to project (I publish tsconfig files, and in some cases I compile TypeScript from node_modules because it's just easy to publish that way and consume them in some build setup designed to work with those files).

@HemalR

This comment has been minimized.

Copy link

HemalR commented Nov 27, 2019

@benjamn Thanks for this! I'm in the process of trying to decipher which package is causing my syntax error on IE:

image

Does the pattern you mentioned allow the names of meteor packages as well as npm packages?

I.e. Could I use this in my package.json:

"meteor": {
    "nodeModules": {
      "recompile": {
        "aldeed:collection2": [
          "client",
          "server"
        ]
      }
    }
  },

At least, the collection2 package is what IE's terrible debugger seems to be pointing to (though I can't see what/why):

image

Ps - If anybody has any tips/tricks on trying to narrow down the package in question that IE doesn't like, then I'd love to hear it.

Thanks in advance

@CaptainN

This comment has been minimized.

Copy link
Contributor

CaptainN commented Nov 27, 2019

I don't think recompile is meant for atmosphere packages, but you do need a setting for simpl-schema. Try:

"meteor": {
    "nodeModules": {
      "recompile": {
        "simpl-schema": "legacy"
      }
    }
  },
@HemalR

This comment has been minimized.

Copy link

HemalR commented Nov 28, 2019

Ok, after many hours yesterday, I decided to try create a new project from scratch to see if I can find it that way. Repo here: https://github.com/HemalR/meteor-ie-breaker/branches

Two branches - the master doesn't work and the 'works' branch does.

The only difference between the two is the presence of the aldeed:collection2 package. I tried adding simpl-schema to the recompile list under package.json to see if that makes a difference, but no luck there.

So that does seem to be the culprit. As I type this, I'm wondering whether the collection2 package is even necessary or whether it has been replaced (don't know why that thought is going through my head). I'll explore and update here if that is the case...

@CaptainN

This comment has been minimized.

Copy link
Contributor

CaptainN commented Nov 28, 2019

There is a bug with the array syntax #10791 try it with the string "legacy" instead.

@HemalR

This comment has been minimized.

Copy link

HemalR commented Nov 28, 2019

@CaptainN YES! That does the trick (on the tester package at least - now to try on production)

@HemalR

This comment has been minimized.

Copy link

HemalR commented Nov 28, 2019

Alright, confirmed that this added to package.json fixes things:

 "meteor": {
    "mainModule": {
      "client": "./client/main.js",
      "server": "./server/main.js"
    },
    "nodeModules": {
      "recompile": {
        "simpl-schema": "legacy",
        "query-string": "legacy"       
      }
    }
  },

@CaptainN - Thank you SO MUCH - I'm well into double digits of hours spent on this. B****y IE!

@CaptainN

This comment has been minimized.

Copy link
Contributor

CaptainN commented Nov 28, 2019

@HemalR You can also just set it to true instead of legacy. That will recompile it for the modern and server bundles too (in case a particular package needs that - Svelte packages generally need that).

@HemalR

This comment has been minimized.

Copy link

HemalR commented Nov 28, 2019

I'm a tad confused by that - the server handles ES6 doesn't it? (And so does Svelte?).

If so, then what is the reasoning for the server option (clearly I'm missing something obvious).

@CaptainN

This comment has been minimized.

Copy link
Contributor

CaptainN commented Nov 28, 2019

The server handles modern syntax, until new syntax is added (maybe like optional chaining or something like that). If a module has that, it won't work without being transpiled. Svelte always needs to be transpiled, since it's a compiler based framework.

@HemalR

This comment has been minimized.

Copy link

HemalR commented Nov 28, 2019

Ah got it, for the Svelte compiler 👍

Svelte is defo on my list of to-try. I imagine it would actually eliminate (or drastically reduce) IE (or any browser specific) issues with any/all packages.

jbl2024 added a commit to jbl2024/latelier that referenced this pull request Jan 12, 2020
@jamesgibson14

This comment has been minimized.

Copy link

jamesgibson14 commented Jan 14, 2020

@benjamn I have a package setup of for recompile and the recompiling is working just fine, however the package imports a few other npm package and the are completely blank with no code, even thought node module is installed. Is there something wrong I am doing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.