search

ProseMirror / prosemirror-markdown

ProseMirror Markdown integration
https://prosemirror.net
MIT License
345 stars 81 forks source link

Transpile module entry with buble #36

Closed marionebl closed 5 years ago

marionebl commented 5 years ago

The intention of this PR is to transpile the module entry via buble the same way the main bundle is.

On our end the introduction of module caused issues with older browsers (namely IE) not supporting used ES2015, this fixes them.

marijnh commented 5 years ago

Do I understand correctly that the problem is that your bundler pulls in the file referenced by the module field instead of the main field, and now your bundle unexpectedly contains ES6 code?

@lpellegr It seems like the addition of a module field can break people's build. Any ideas for a solution?

lpellegr commented 5 years ago

I am a bit confused. To my knowledge, module imports are an ES6 (=ES2015) feature:

https://github.com/rollup/rollup/wiki/pkg.module

As a consequence, I don't see why the module field should reference an ES5 file.

@marionebl It's not clear to me why your build is picking the source from the module field. Is the issue only with prosemirror-markdown submodule or also with others? Would it be possible to share a minimal, reproducible example?

PS: prosemirror-markdown and prosemirror-menu include respectively markdown-it and crel versions that do not support ESM so it's understandable that ES6+ builds making use of these packages are getting errors.

PS2: @marijnh my PR on crel has been merged, I will submit a patch for prosemirror-menu once a new crel release is available.

marijnh commented 5 years ago

As a consequence, I don't see why the module field should reference an ES5 file.

Because people are using bundlers that prefer the module field to build their code, which is causing ES6 code to be included where before you only got ES5 code. The addition of a module field thus changes the behavior of builds in a way that breaks things for existing users.

lpellegr commented 5 years ago

but in that case, isn't the bundler or its configuration to be the issue?

marijnh commented 5 years ago

That's one way to view this.

@marionebl I think for the time being, a better solution would be for you to run an es5 compiler over your code in your own build process. Depending on how many other people run into this, I might reconsider whether the prosemirror packages should be changed, but there's quite a lot of other packages whose module field points at ES6 code, so the expectation that packages always yield ES5 code isn't a solid one.

ocavue commented 5 years ago

isn't the bundler or its configuration to be the issue?

Base on what I saw, module is only a proposal.

webpack docs The key main refers to the standard from package.json, and module to a proposal to allow the JavaScript ecosystem upgrade to use ES2015 modules without breaking backwards compatibility.

https://github.com/rollup/rollup/wiki/pkg.module (Note: Tools such as rollup-plugin-node-resolve and Webpack 2 treat jsnext:main and module interchangeably. They're the same thing, except that module is more likely to become standardised.)

lpellegr commented 5 years ago

@ocavue Yes, that what was discussed in prosemirror forum before implementing the change. The module is only a proposal but used by a large ecosystem. Also, as you quoted, it aims:

to allow the JavaScript ecosystem upgrade to use ES2015 modules without breaking backwards compatibility

@marijnh It could make sense to remove the module field for this module (prosemirror-markdown) until an ESM version of markdown-it is used since all ESM build that include this PM submodule will fail. As already discussed, I maintain a fork for an ESM compatible version.

marionebl commented 5 years ago

@marionebl It's not clear to me why your build is picking the source from the module field.

We are using webpack which defaults to preferring module over main (webpack docs), presumably to allow for more effective tree shaking, which depends on ES2015 module syntax.

Is the issue only with prosemirror-markdown submodule or also with others?

Probing into our git history we had this issue with other deps in the past. Mostly this was resolved by convincing library authors to ship es module syntax with the rest of ES2015 transpiled down to ES5 via module.

Would it be possible to share a minimal, reproducible example?

https://github.com/marionebl/prosemirror-markdown-bundle-repro

marionebl commented 5 years ago

@marionebl I think for the time being, a better solution would be for you to run an es5 compiler over your code in your own build process. Depending on how many other people run into this, I might reconsider whether the prosemirror packages should be changed, but there's quite a lot of other packages whose module field points at ES6 code, so the expectation that packages always yield ES5 code isn't a solid one.

Ok, we certainly can do that. Maybe you could factor this into your decision, as the intent behind module was different according to the proposal:

If you're using things like arrow functions and classes (or more exotic features like decorators and object spread), you should transpile them if you want your library to work in environments that don't support those features.

https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features

marijnh commented 5 years ago

See also prosemirror/prosemirror#1003. I'm adding a compiled-down ES module file to the build process of all packages (implemented in 94de5bc847d04bd in this repository).