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

Meteor should transpile js in node_modules #6

Closed
mwarren2 opened this issue Jun 6, 2017 · 31 comments
Closed

Meteor should transpile js in node_modules #6

mwarren2 opened this issue Jun 6, 2017 · 31 comments
Assignees
Milestone

Comments

@mwarren2
Copy link

@mwarren2 mwarren2 commented Jun 6, 2017

Migrated from: meteor/meteor#8640

@trusktr
Copy link

@trusktr trusktr commented Jun 6, 2017

Maybe this can be somehow configurable. There are indeed packages that ship original source files written in ES6 module format (and using new language features). These packages usually also include transpiled "dist" files.

One advantage of having this is that if someone wants to make an NPM package just for Meteor, they don't need to worry about setting up Webpack/Babel/etc, they can just write code, publish it, say that it works in Meteor, and forget about it, without having to worry about tooling... because Meteor already has all the tooling.

@ejfrancis
Copy link

@ejfrancis ejfrancis commented Jun 6, 2017

Perhaps we could do something similar to what rollup does where you can specify a "module" main in your package.json which is a non-transpiled es2015 module?

@joncursi
Copy link

@joncursi joncursi commented Jun 13, 2017

I hit this issue ~50% of the time I install a new module from npm, which forces me to hunt for an alternate version to specifically make Meteor happy. Other projects I'm working on (based on Webpack, React Native, etc.) run these node_modules without a hitch. It's starting to get pretty painful.

It seems like a lot of modules, especially those in the react-native-* world (i.e. react-native-vector-icons, react-native-elements) are using more advanced JS standards, and Meteor doesn't know what to do with it. Browser console throws:

Uncaught SyntaxError: Unexpected token import

What can we do to fix this? Can Meteor do some additional transpilation on the bundle before sending to the browser?

@benjamn
Copy link
Member

@benjamn benjamn commented Jun 13, 2017

@joncursi So those npm packages don't work in any version of Node without transpilation? That seems like a strange choice for them to be making!

While I can certainly feel your pain, you're attempting (if I understand your project) to reuse code designed to work specifically with React Native in Meteor projects. I would love to make that easier, if possible, but the rate at which you encounter this problem is definitely not representative of most of the npm ecosystem, nor of most Meteor apps. Just something to bear in mind.

@benjamn
Copy link
Member

@benjamn benjamn commented Jun 13, 2017

@ejfrancis The module field is supposed to identify to code that has been completely transpiled except for import and export declarations, so that idea might help if we started transpiling just import and export in node_modules.

The last time I looked into that, the problem was that other CommonJS modules from npm don't know how to deal with what's exported by the ECMAScript module source tree (specifically the .default property). And of course using the main entry point for CommonJS but the module entry point for ECMAScript is a bad idea because then you get two different copies of the package depending on who's importing it, with different shared state.

Using the module field would not help with packages using syntax targeting Node versions more recent than Node 4, such as async/await. I think Meteor 1.6 (Node 8) will be a better solution to that problem than transpilation, in the long term.

@trusktr
Copy link

@trusktr trusktr commented Jun 13, 2017

I think Meteor 1.6 (Node 8) will be a better solution to that problem than transpilation, in the long term.

But there's also client code and supporting older browsers. It may be possible some NPM module is designed for browsers, but the code published is for too recent of a browser than supported by the app dev.

What about some sort of configuration json file that Meteor modules/ecmascript can read, that defines certain node modules that should be transpiled just like other non-node_modules code in the app?

@benjamn
Copy link
Member

@benjamn benjamn commented Jun 14, 2017

@trusktr That's a good point. I think the only configuration we should need is the "engines" field of the package's package.json file.

@trusktr
Copy link

@trusktr trusktr commented Jun 14, 2017

That would be ideal if package authors were willing to put certain fields in package.json. The reality is: it is unrealistic to expect that every package author will put every required field for every possible target environment into package.json.

It would be great for a Meteor app dev to be able to apply a setting without relying on a package dev having to do it, and without having to fork a package just to add the setting.

@benjamn
Copy link
Member

@benjamn benjamn commented Jun 14, 2017

I'm comfortable saying that you (an npm package author) have to specify engines if you want any special treatment of new syntax. In most cases that I've seen where an npm package uses syntax not supported by the current LTS version, there's an appropriate engines field in package.json, and it is a perfectly reasonable thing to ask the package author to change.

@mwarren2
Copy link
Author

@mwarren2 mwarren2 commented Jun 17, 2017

@benjamn After reading your technical comments, which are a little beyond me, I hope that my use case, which I detailed in the original issue before turning it into a feature request, is still relevant.

Just to sum up, my meteor builds are failing when trying to install quasar-framework (a vue-based ui framework) from npm.
The .js files in the quasar-framework /dist folder have been converted and have no imports.
The build is failing on imports from .vue files while apparently compiling from the /src folder.
There's an example test case in the original post.

If I'm barking up the wrong tree by introducing the problem in this feature request please set me right.

@ghybs
Copy link

@ghybs ghybs commented Jun 26, 2017

Hi @mwarren2,

I am not sure how just npm installing quasar-framework makes your example app fail, without even trying to import it (maybe that is how the Atmosphere package akryum:vue works?)

Anyway, there is a demo Meteor boilerplate from Quasar Framework:
https://github.com/quasarframework/quasar-template-meteor

If I understand correctly, for now they have just copied their es6 dist file into the imports folder and you then import it on the Client.

Hope this helps.

@mwarren2
Copy link
Author

@mwarren2 mwarren2 commented Jun 26, 2017

@ghybs Thanks. However the Meteor boilerplate at quasar-template-meteor is mine, I've been working for a while with quasar-framework's creator to get it working with Meteor from npm.

@trusktr
Copy link

@trusktr trusktr commented Jun 27, 2017

@mwarren2
Copy link
Author

@mwarren2 mwarren2 commented Jun 28, 2017

@trusktr, @benjamn Ah, it appears there is a solution to my particular problem, from akryum:vue-component

The solution is here and is as follows:

Add a .vueignore file in the project root with the following content:

node_modules/

I can now build my test app, so my case is solved.

@aliogaili
Copy link

@aliogaili aliogaili commented Aug 27, 2017

I've ran into many issues with NPM packages using ES6 syntax when running the app on older browsers.

Any workaround for this?

@benjamn benjamn added this to the Meteor 1.6.1 milestone Nov 1, 2017
@lukejagodzinski
Copy link

@lukejagodzinski lukejagodzinski commented Nov 2, 2017

@benjamn today I wrote a post on Meteor forums about things related to this topic.

I think that Meteor should transpile some of the packages in the node_modules directory if they contain ES imports/exports. The good example of a package that is using ES imports/exports is lodash-es. We should try to embrace new ES features and encourage people to switch to the new syntax. Currently it's also possible to use ES imports/exports in Node thanks to @std/esm.

In essence, I wouldn't want to wait until 2018 (or even 2020) when ES import/export syntax will be available in Node and all browsers

@benjamn
Copy link
Member

@benjamn benjamn commented Nov 2, 2017

To be completely honest, the main reasons we don't currently compile modules in node_modules are (1) performance (it would take time, obviously), and (2) back in the Meteor 1.2 days, when ecmascript was first introduced, it seemed reasonable to assume that packages published to npm would be compiled down to ES5 first.

We've always known that this assumption would become less and less reasonable over time, as package authors began to publish newer syntax to npm, and at some point we would have to start compiling code from npm, at least for the browser. Node 8 handles almost everything natively (except for ECMAScript module syntax), but that only helps on the server.

Now that Meteor 1.6 is out, I think this issue deserves top priority for Meteor 1.6.1, which is why I put it in the milestone. Even compared to the other feature requests currently in the milestone, I think this feature is by far the most important.

At the very least, we have to start compiling any modules that we include in the client bundle. This is complicated for some obscure legacy reasons, but that doesn't matter. We can rewrite the problematic code from scratch, if necessary.

Ideally, I would love to reconsider the dialect of ECMAScript that compiler plugins in Meteor (not only ecmascript but also coffeescript, barbatus:typescript, etc.) should be targeting when generating compiled code. Right now it's ES5 + CommonJS require and exports. I would be much happier if that also included import and export syntax, because then compiler plugins would no longer be responsible for compiling that syntax, and we could implement fancy optimizations like tree shaking in Meteor core, rather than forcing plugins to understand inter-module dependencies.

Performance remains a concern, but Node 8 is noticeably faster, Babel is getting faster all the time, and of course we can cache the compiled code aggressively.

Thanks to everyone for keeping this issue active. I look forward to making progress on this soon, because I agree it's long overdue.

@lukejagodzinski
Copy link

@lukejagodzinski lukejagodzinski commented Nov 2, 2017

@benjamn great to hear that! I think it's definitely the right time to do it. I wouldn't be concerned a lot about performance because as you said we can always make use of the cache. The initial load might take more time but later it will be rather not very common to recompile everything. I would only be worried about tree shaking implementation but Rollup does it (or at least it's what they say in documentation).

If it goes about compilers, I see that for example, the typescript compiler has an option to customize target ES version per project https://github.com/barbatus/typescript#es6 that way developers can just choose.

@robinknox
Copy link

@robinknox robinknox commented Dec 15, 2017

Another use case for this is the react-facebook NPM package which currently contains some ES6 https://github.com/seeden/react-facebook meaning that I can't currently use it on the clientside.

@trusktr
Copy link

@trusktr trusktr commented Jan 6, 2018

@benjamn

I am assuming that nothing in node_modules will be compiled by default, and that this new feature will be an opt-in in feature.

Have you already come up with some ideas on what the API will look like for opting into transpiling certain packages in node_modules?

@trusktr
Copy link

@trusktr trusktr commented Jan 6, 2018

Would a it make sense to have an option for specifying that transient dependencies of a targeted package should also be transpiled?

Manually having to specify various dependency packages of a main package the the app depends on might get tedious, but it would lead to the best performance. In many cases, just specifying for Meteor to transpile a package's whole dependency tree would be easier and perfectly fine if the build time difference is tolerable.

@dbx834
Copy link

@dbx834 dbx834 commented Feb 20, 2018

Here is another use-case:

Meteor doesn't work so well at the moment with Box, specifically box-ui-elements and box-react-ui.

Maybe it's a good idea if it can be controlled what packages in node_modules will be transpiled rather than transpiling everything...

PS: I've put in a request to them to include ES5 in their distribution - box/box-ui-elements#184.

benjamn added a commit to meteor/meteor that referenced this issue Mar 23, 2018
If a package in node_modules needs to be compiled for older browsers,
simply symlink the package directory into your application somewhere, and
then import the package as you normally would.

Because of the symlink, code within the package will be compiled as if it
was part of your application, and any imports that refer to modules in the
package will automatically use the compiled code rather than the raw code
from node_modules.

Note that you can also symlink individual files to make them be compiled
like application modules, rather than linking an entire package directory.

Creating symlinks could be considered a form of configuration, but
otherwise this is a zero-configuration solution to selectively compiling
packages within node_modules, which has been something of a holy grail in
the JavaScript community lately.

meteor/meteor-feature-requests#6
benjamn added a commit to meteor/meteor that referenced this issue Mar 23, 2018
If a package in node_modules needs to be compiled for older browsers,
simply symlink the package directory into your application somewhere, and
then import the package as you normally would.

Because of the symlink, code within the package will be compiled as if it
was part of your application, and any imports that refer to modules in the
package will automatically use the compiled code rather than the raw code
from node_modules.

Note that you can also symlink individual files to make them be compiled
like application modules, rather than linking an entire package directory.

Creating symlinks could be considered a form of configuration, but
otherwise this is a zero-configuration solution to selectively compiling
packages within node_modules, which has been something of a holy grail in
the JavaScript community lately.

meteor/meteor-feature-requests#6
benjamn added a commit to meteor/meteor that referenced this issue Mar 26, 2018
If a package in node_modules needs to be compiled for older browsers,
simply symlink the package directory into your application somewhere, and
then import the package as you normally would.

Because of the symlink, code within the package will be compiled as if it
was part of your application, and any imports that refer to modules in the
package will automatically use the compiled code rather than the raw code
from node_modules.

Note that you can also symlink individual files to make them be compiled
like application modules, rather than linking an entire package directory.

Creating symlinks could be considered a form of configuration, but
otherwise this is a zero-configuration solution to selectively compiling
packages within node_modules, which has been something of a holy grail in
the JavaScript community lately.

meteor/meteor-feature-requests#6
@benjamn
Copy link
Member

@benjamn benjamn commented Mar 26, 2018

We think we've finally found a way to solve this problem: meteor/meteor#9771

tl;dr Use symbolic links to expose node_modules code within your application, outside of node_modules. Meteor will compile the exposed code as if it was part of your application, using whatever compiler plugins you have installed, and also guarantee that you get the compiled code when you import from node_modules (this is they key new feature).

@CaptainN
Copy link

@CaptainN CaptainN commented Mar 27, 2018

That feature looks sweet!

Can we make it even easier? For example - a set of config options in package.json (in the meteor tree) that would allow a developer to cherry pick the packages they want without having to create local symlinks or specifically clone a repo? We could even allow a basic set of package reconfiguration (to allow the user to select from the src directory instead of dist). Something like:

{
  "meteor": {
    "mainModule": {
      "client": "client/main.js",
      "server": "server/main.js"
    },
    "modernModules": {
      "package-name": {
        "main": "src/main.js"
      }
    }
  }
}

Basically, that setting could create the symlinks from config at compile time, instead of requiring the user to set those up (may be easier especially on Windows).

@benjamn
Copy link
Member

@benjamn benjamn commented Mar 27, 2018

@CaptainN Honestly I think the main use case will be cloning the package repository locally and then running npm install path/to/package to link it into node_modules, which doesn't require the developer to make any explicit symbolic links.

I would rather not invent new configuration options that are less powerful than the "real" way of doing this, which may involve modifying the package locally, adding your own .babelrc file, etc. When you start to think about all the possible dimensions of these modifications, coming up with a package.json-based configuration system that does something equivalent (and teaching everyone how to use it) feels pretty daunting.

That said, if folks really need the new functionality to be simpler, we can maybe consider implementing another form of configuration in the future.

@CaptainN
Copy link

@CaptainN CaptainN commented Mar 27, 2018

@benjamn That's true. It's actually similar to how I manage certain Meteor packages.

I wonder whether there are enough packages in npm that could benefit from simply using the ES6+ modules directly (and getting the benefits of Meteor's modern/legacy builds) rather than always using what are essentially legacy builds in the main prop/dist folders of many npm packages.

You are probably right though - it may not be as simple in most cases as defining the entry point in an existing npm module, which is the extent of flexibility I was thinking about with regard to configuration (basically, an alternative to symlinks, with one additional config option).

@mwarren2
Copy link
Author

@mwarren2 mwarren2 commented Jun 13, 2018

@benjamn My original reason for opening this issue was due to problems with quasar-framework, which I had to transpile manually, and this has now been solved.

meteor/meteor#9771 has elegantly dealt with the problem, which I solved by linking to the node_modules installation from the /imports folder, and employing setMinimumBrowserVersions(), as shown in the documentation for Meteor 1.7 .

Thank you for all your work on this.

As far as I am concerned this issue can be closed.

@benjamn
Copy link
Member

@benjamn benjamn commented Jun 13, 2018

@mwarren2 That's great to hear! Feel free to comment here in the future if you run into any problems with this workflow.

@yanickrochon
Copy link

@yanickrochon yanickrochon commented Nov 14, 2019

I have run into this problem with a few npm packages lately, big packages like TypeORM and BabylonJS, etc. With JS features growing, and the general acceptance of import/export and other ES6+ features, it seems like holding back on this seems a hacky approach.

I would love to have an option to manually specify which npm module should be included in the build process. I don't know, something like .meteor.conf such as

{
   "build": {
      "includeModules": [
         "typeorm",
         [ "@babylonjs/core", { "target": [ "browser" ] } ]
      ]
   }
}

Some packages, such as TypeORM does not have an already transpiled release, and this makes for a hacky solution, as documented earlier.

As I have other projects using react-scripts, which does compile node_modules with no issue, this is a problem that can be fixed. As other package maintainers opine, it is not the package owners to decide how their packages should be compiled, and with what compatibility layers it should have; each project is different, and each project should optimize transpiling for their own needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.