move to ESM and what not #1349

43081j · 2018-10-02T22:04:38Z

👋

I had this lying around and decided to finish it off recently in my fork as it was needed for trying out in some web component stuff I had setup.

Fixes #1288

Is this something you'd be interested in? Happy to keep it in my fork if not and close this PR.

Here's a summary of what happened in this branch:

Sources separated into modules
All sources are valid ESM (src/*)
Moved from prototypical concepts to classes
Introduced babel to transpile to CJS modules (lib/*) for when node users require this
Introduced rollup to bundle to a browser-compatible library (marked.min.js) which behaves the same as the original
Introduced files to package.json so npm pack and publish will package only bin/lib/man (CJS sources)
Introduced a module entry to package.json so bundlers can resolve the ESM sources
Replaced all unscoped vars and what not to let/const

All tests pass, build succeeds.

Have not been using it in a browser myself so it seems that would need further testing.

Whats missing:

The browser bundle isn't minified (something like babel-preset-minify could achieve that)
Performance comparison
Build will fail on CI as npm run build needs adding to travis.yml

Again, if this is of any use to you just let me know and ill get back on it.

cc @joshbruce

.npmignore

package.json

styfle · 2018-10-03T13:24:10Z

package.json

@@ -51,11 +60,15 @@
    "test:lint": "eslint bin/marked .",
    "test:redos": "eslint --plugin vuln-regex-detector --rule '\"vuln-regex-detector/no-vuln-regex\": 2' lib/marked.js",
    "bench": "node test --bench",
-    "lint": "eslint --fix bin/marked .",
-    "build": "uglifyjs lib/marked.js -cm  --comments /Copyright/ -o marked.min.js",


Where are you generating marked.min.js?

the rollup call produces it, unminified at the min but introducing babel-preset-minify should sort that. inside the build script further up

styfle · 2018-10-03T13:30:26Z

The tests are failing with Error: Cannot find module './lexer'

43081j · 2018-10-03T13:36:55Z

Yup this is because the travis config needs updating to npm run build beforehand.

styfle · 2018-10-03T14:29:10Z

@43081j You should be able to modify .travis.yml and add a build step

43081j · 2018-10-03T14:30:47Z

Yup its part of the list in my original post of whats left to do. simple enough, just didn't get around to it

styfle · 2018-10-03T14:34:36Z

Excellent! Let's wait to see what the rest of the team thinks.

UziTech · 2018-10-03T18:24:10Z

This is great! However I think we were waiting to remove older node versions until we get all the specs passing. probably wait for v1.x

UziTech · 2018-10-03T18:46:26Z

.travis.yml

@@ -7,13 +7,12 @@ jobs:

  include:
    - stage: unit tests 👩🏽‍💻
-      script: npm run test:unit
+      script: npm run build && npm run test:unit


since npm test already includes npm run build it is redundant here and below

UziTech · 2018-10-03T18:48:38Z

package.json

    "bench": "node test --bench",
-    "lint": "eslint --fix bin/marked .",
-    "build": "uglifyjs lib/marked.js -cm  --comments /Copyright/ -o marked.min.js",
    "preversion": "npm run build && (git diff --quiet || git commit -am 'minify')"
  },
  "engines": {
    "node": ">=0.10.0"


should this be updated to ">=6.0.0"?

UziTech · 2018-10-03T18:52:02Z

Do we need to add browserslist to package.json for babel-preset-env?

43081j · 2018-10-03T18:58:48Z

We probably should but i wasn't too sure what the correct targets should be.

The ESM sources account for any browsers supporting ES modules.

The CJS sources should only be used by node, so our babel-preset config should probably be node: true.

The bundle should be consumed by non-ESM browsers so its actually a different target to lib/ I guess (overridable via rollup.config.js if we must). We would have to find the minimum browser version we could support.

UziTech · 2018-10-03T20:03:07Z

package.json

-    "test:lint": "eslint bin/marked .",
-    "test:redos": "eslint --plugin vuln-regex-detector --rule '\"vuln-regex-detector/no-vuln-regex\": 2' lib/marked.js",
+    "test:lint": "eslint bin/marked \"test/**/*.js\"",
+    "test:redos": "eslint --plugin vuln-regex-detector --rule '\"vuln-regex-detector/no-vuln-regex\": 2' src/marked.js",
    "bench": "node test --bench",


"test:old" and "bench" will need to be prepended with npm run build &&

UziTech · 2018-10-03T20:05:04Z

rollup.config.js

+    name: 'marked'
+  },
+  plugins: [babel()]
+}


Missing semicolon (sorry but my IDE nags me 🙃)

UziTech · 2018-10-03T20:35:18Z

benchmarks seem to be consistent 👍:

// master
marked completed in 5809ms.
marked (gfm) completed in 7043ms.
marked (pedantic) completed in 5590ms.
commonmark completed in 8877ms.
markdown-it completed in 6774ms.
markdown.js completed in 22230ms.

// this pr
marked completed in 6027ms.
marked (gfm) completed in 6981ms.
marked (pedantic) completed in 5792ms.
commonmark completed in 9057ms.
markdown-it completed in 8349ms.
markdown.js completed in 20882ms.

joshbruce · 2018-10-16T02:21:22Z

With @UziTech on waiting for 1.x - what implications are there for us continuing to push toward spec compliance and this PR becoming conflicted?

Is there a way to have our cake and eat it too? Probably not in this case, just want to keep our promise unless the community says it's cool to jump some steps. We haven't visited the roadmap in a minute.

43081j · 2018-10-16T08:52:53Z

What sort of time frame would you be looking at for 1.x?

To be honest, ES modules are still a bit flaky with node which is why we're forced to have 3 builds (ESM, CJS, browser).

I think if we try wait for node to have a better implementation, we'll be here a while.

It may be worth us looking into using this:
https://github.com/standard-things/esm

instead of shipping two copies of the sources 🤔 but its a fairly big dependency to be shipping i guess

styfle · 2018-10-16T14:17:59Z

I looked this over again and I am becoming more reserved the more I look at it.

ES modules are still a bit flaky with node which is why we're forced to have 3 builds (ESM, CJS, browser).

This PR is adding more complexity to a relatively small project and I'm not sure the value added here is worth it at this time.

Let's do a pros/cons list

Pros

Developers get to use multiple files - maybe avoid merge conflicts?
Developers get to use ES6 syntax - less writing and more readable in most cases
(maybe) Users get to use ESM sources with bundlers treeshake for smaller output (not sure what would be removed in this case)

Cons

The ESM source will not run in the browser as-is (due to bare specifiers)
- Might get solved by package-name-maps proposal but it's not finalized
The ESM source will not run in Node.js as-is (due to file extension)
- Today, this requires running node with a flag and changing the extension to .mjs
The bundle size will be larger by a few KB (was 42 KB and now 47 KB) but minified size is yet to be determined
We'll be publishing more sources files so removing those sources is a breaking change (probably can do this in 1.x)
More complexity due to more build tools requires maintenance when upgrading versions: babel, rollup, and several more plugins
(maybe) Some js user's don't know ES6 syntax and won't be able to contribute
(maybe) Build time will increase slightly due to more build targets
(maybe) Git history/blame will be harder to look through since file is being split
(maybe) Any existing PRs (currently 18) will all get merge conflicts as soon as this lands

joshbruce · 2018-10-16T14:49:44Z

@styfle brings up some very good points.

Tagging #746 as it describes a method for modularizing the codebase with straight ES5 or 6 (??) (doing this off the top of my head).

To @43081j's question, the core team is really trying to get the community to step up more. One of the issues of the past (before the current core team) was an expectation became set that the core team would do everything...so I cannot tell you how awesome it is to have you post this PR, whether it's accepted or not...anyhoo, the core team primarily sets direction, creates tools, and ensures consistency of the codebase and approach; so, all that to say, timeframe is up to the broader community as the core team will focus more on helping and coordination than doing (though we do some of that when we can). A refrain that became popular for a minute was "1.0 could happen tomorrow or may never happen, it's up to the community" (or something like that).

See #1106 for the details and drama...in fact, probably time to write another one.

Throwing copper: I think, based on @styfle's thoughts, we should close this for now - reference to what I think is the main modularization issue - and maybe resurrect at that time. Spec-compliance and keeping the infrastructure clean per the roadmap and milestones (https://github.com/markedjs/marked/milestones) unless we find something that can minimize impact on legacy users or out right fake backward compatibility. Not to say the new stuff isn't nice, just may not be ready to be part of main..

43081j · 2018-10-16T14:54:48Z

This is meant to introduce the ability to use the ES modules in browsers as is. So if that means I forgot to (and need to) add the file extension to each import, I should.

The bundle size is of course larger, it isn't even minified yet (and rollup will tree-shake, which it hasn't done yet either).

The rest of @styfle's points are fairly small IMHO, tooling being introduced isn't a problem, nor is build time (it won't be noticeable most likely).

From a quick look at your milestones, I can't see how this project can ever reach a point where it keeps up. If 1.x is far away and isn't even a large jump from what you have now, how far away is 2.x? A great problem these days is that projects, like this, could easily support ES modules so browsers can use them as-is, but don't.

At the cost of some tooling, you gain a much cleaner codebase which is far more maintainable and git-friendly (who wants a monolithic file checked into git?). I admit having sources and two outputs (CJS, bundle) is not ideal, but that's something to discuss rather than something to assume is the only solution.

Martii · 2018-10-16T22:50:31Z

Developers get to use ES6 syntax - less writing and more readable in most cases

This is actually a con in our book. ES6 may be supported however it's very subjective on readability. Examples written in ES5 are a good base for projects that don't use ES6+ and extending it to the more quirky ES6+ syntaxes.

move to ESM and what not

6cc05c7

styfle reviewed Oct 3, 2018

View reviewed changes

.npmignore Show resolved Hide resolved

styfle reviewed Oct 3, 2018

View reviewed changes

package.json Show resolved Hide resolved

styfle reviewed Oct 3, 2018

View reviewed changes

styfle requested review from UziTech, joshbruce and davisjam October 3, 2018 13:32

43081j added 4 commits October 3, 2018 18:48

fix test linting and travis config

eb767e4

build during test run

abcf280

remove old node

4455a2d

pass through babel during bundling

7a2ee5c

UziTech reviewed Oct 3, 2018

View reviewed changes

remove redundant build

e98560c

UziTech reviewed Oct 3, 2018

View reviewed changes

fix rollup config lint

c3a4008

styfle mentioned this pull request Oct 4, 2018

ESM (ES6 module) distribution #1288

Closed

tmcw mentioned this pull request Mar 21, 2019

feat: ES Module entry point #1452

Closed

4 tasks

UziTech mentioned this pull request Mar 22, 2019

Why isn't marked.js broken down into multiple files? #1453

Closed

saschanaz mentioned this pull request Apr 7, 2019

refactor: dynamic import for assets speced/respec#2233

Closed

UziTech mentioned this pull request Aug 22, 2019

Modernizing the codebase #1539

Closed

UziTech mentioned this pull request Nov 5, 2019

Separate marked into modules #1563

Merged

7 tasks

UziTech closed this in #1563 Dec 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move to ESM and what not #1349

move to ESM and what not #1349

43081j commented Oct 2, 2018 •

edited by styfle

Loading

styfle Oct 3, 2018 •

edited

Loading

43081j Oct 3, 2018 •

edited

Loading

styfle commented Oct 3, 2018

43081j commented Oct 3, 2018

styfle commented Oct 3, 2018

43081j commented Oct 3, 2018

styfle commented Oct 3, 2018

UziTech commented Oct 3, 2018

UziTech Oct 3, 2018

UziTech Oct 3, 2018

UziTech commented Oct 3, 2018

43081j commented Oct 3, 2018

UziTech Oct 3, 2018

UziTech Oct 3, 2018

UziTech commented Oct 3, 2018

joshbruce commented Oct 16, 2018

43081j commented Oct 16, 2018

styfle commented Oct 16, 2018 •

edited

Loading

joshbruce commented Oct 16, 2018

43081j commented Oct 16, 2018 •

edited

Loading

Martii commented Oct 16, 2018

move to ESM and what not #1349

move to ESM and what not #1349

Conversation

43081j commented Oct 2, 2018 • edited by styfle Loading

styfle Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

43081j Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

styfle commented Oct 3, 2018

43081j commented Oct 3, 2018

styfle commented Oct 3, 2018

43081j commented Oct 3, 2018

styfle commented Oct 3, 2018

UziTech commented Oct 3, 2018

UziTech Oct 3, 2018

Choose a reason for hiding this comment

UziTech Oct 3, 2018

Choose a reason for hiding this comment

UziTech commented Oct 3, 2018

43081j commented Oct 3, 2018

UziTech Oct 3, 2018

Choose a reason for hiding this comment

UziTech Oct 3, 2018

Choose a reason for hiding this comment

UziTech commented Oct 3, 2018

joshbruce commented Oct 16, 2018

43081j commented Oct 16, 2018

styfle commented Oct 16, 2018 • edited Loading

Pros

Cons

joshbruce commented Oct 16, 2018

43081j commented Oct 16, 2018 • edited Loading

Martii commented Oct 16, 2018

43081j commented Oct 2, 2018 •

edited by styfle

Loading

styfle Oct 3, 2018 •

edited

Loading

43081j Oct 3, 2018 •

edited

Loading

styfle commented Oct 16, 2018 •

edited

Loading

43081j commented Oct 16, 2018 •

edited

Loading