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

feat: ES Module entry point #1452

Closed
wants to merge 1 commit into from
Closed

feat: ES Module entry point #1452

wants to merge 1 commit into from

Conversation

tmcw
Copy link

@tmcw tmcw commented Mar 21, 2019

BREAKING CHANGE: some module loaders will automatically take the
module entry point when they previously loaded main.

This adds a rollup dependency to produce a UMD bundle, like in previous
marked releases, and allows marked to be written and consumed as an ES
Module.

Fixes #1288


This does the same thing as #1349, but:

  • Instead of jumping into ES6 syntax as well as ES Module syntax at the same time, this just introduces ES Module syntax.
  • Uses rollup instead of babel. It's about transforming the ES Module syntax, and for that it's radically simpler.
  • Minimizes change otherwise. Doesn't split up files. Should not botch many PRs inflight, or at least they should be rebase/rewritable without much pain.
  • Uses .mjs to let Node with --experimental-modules work for now. This could be changed if folks don't care too much about Node compatibility.
  • lib/marked.js is now a generated file and is regenerated when you test or publish.

Note that this requires a major version bump, or at least a big warning. I wrote about this here, and it's essentially unavoidable because of Webpack's rather… questionable… choice of behavior.

Description

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@UziTech
Copy link
Member

UziTech commented Mar 21, 2019

It is going to break a lot of stuff if our main entry point isn't /lib/marked.js. Anyone who uses jsdelivr and doesn't use the minified file will break the instant this PR is merged. This might be something we will do in v2.0 after deprecating things in v1.0.

Would it be possible to just add /lib/marked.mjs that just "imports" marked.js so marked.js is left alone?

@tmcw
Copy link
Author

tmcw commented Mar 21, 2019

It is going to break a lot of stuff if our main entry point isn't /lib/marked.js.

This PR doesn't change the main entry point - /lib/marked.js is still in the distribution and is still main, it's only ignored with .gitignore because it's a generated file. This adds the module entry point.

Would it be possible to just add /lib/marked.mjs that just "imports" marked.js so marked.js is left alone?

Not exactly - ES Module syntax is not a superset of (CommonJS + ES Module), it's just ES Module syntax. So if /lib/marked.js was CommonJS and was included with require() and exported with export, it wouldn't improve the user experience for any particular consumers of marked and would likely break some module loaders.

@UziTech
Copy link
Member

UziTech commented Mar 21, 2019

it's only ignored with .gitignore because it's a generated file

since it is in .gitignore it won't show up on any site using marked on the client side with:

<script src="https://cdn.jsdelivr.net/gh/markedjs/marked/lib/marked.js"></script>

@UziTech
Copy link
Member

UziTech commented Mar 21, 2019

That is the reason marked.min.js is not ignored as well.

@tmcw
Copy link
Author

tmcw commented Mar 21, 2019

Okay, ignoring generated files is my custom but isn't a requirement. I've pushed an update that re-includes lib/marked.js.

@UziTech
Copy link
Member

UziTech commented Mar 21, 2019

Could we make \lib\marked.mjs the generated file?

@tmcw
Copy link
Author

tmcw commented Mar 21, 2019

Not with any out-of-the-box solution, as far as I can tell. Detecting & transforming a UMD export is a lot less certain than detecting & transforming an ES Module.

@TrySound
Copy link

mjs in module field is broken in webpack. Webpack is quite opinionated about it. Since it's not a requirement for esm in node I would suggest to not use mjs at all and go with .esm.js suffix instead.

BREAKING CHANGE: some module loaders will automatically take the
module entry point when they previously loaded main.

This adds a rollup dependency to produce a UMD bundle, like in previous
marked releases, and allows marked to be written and consumed as an ES
Module.
@tmcw
Copy link
Author

tmcw commented Mar 21, 2019

Webpack has such unique opinions. Switched to .esm.js.

@styfle
Copy link
Member

styfle commented Mar 21, 2019

I say we wait until Node gets ESM. Right now, the API is in flux and its best to wait until things settle down.

Then we can actually run tests in CI against the ESM code.

@TrySound
Copy link

module field is not related to node way to use esm. It's useful for bundlers at least.

@styfle
Copy link
Member

styfle commented Mar 22, 2019

I think I still stand by these comments from a previous PR: #1349 (comment)

@tmcw
Copy link
Author

tmcw commented Mar 22, 2019

Okay - it's up to the project maintainers for a final decision, but I should point out that this PR addresses many of the concerns.

It doesn't switch to ES6 syntax everywhere, it requires one additional dev dependency (instead of 7), there's one build target and rollup is super fast, there's net zero size change (instead of 5kb), and it'll run in a browser everywhere and unmodified (because it's a single-file source, so module specifiers don't matter).

@Andarist
Copy link
Contributor

Unfortunately, after I've analyzed the exported module shape I've come to the conclusion that this cannot be done without breaking change because of webpack's broken interop between cjs/esm formats - webpack/webpack#7973

I would very much see this refactored to ESM and to a tree-shakeable form of exports, but that's up to Marked team. Just 2 add my 2 cents - while changes around this might be "breaking" they are not functional changes, the only way that would have to change is the exported shape. Bumping a minor (as marked still is in its 0.x versions) should be sufficient to introduce any breaking changes - instead of stalling forever it's IMHO better to push such things forwards for the greater good.

@UziTech UziTech mentioned this pull request Nov 5, 2019
7 tasks
@UziTech UziTech closed this in #1563 Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM (ES6 module) distribution
5 participants