fix: fix node v12 #2109

UziTech · 2021-06-16T04:55:15Z

Marked version: 2.1.0

Description

remove optional chaining to fix node v12 support

update engines to support node v12

test on node v12

Fixes Breaking change in 2.1.0 #2106

Contributor

Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
no tests required for this PR.
If submitting new feature, it has been documented in the appropriate places.

Committer

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

CI is green (no forced merge required).
Squash and Merge PR following conventional commit guidelines.

vercel · 2021-06-16T04:55:19Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/56pQ5vXq6RhaF5HDwsi6CuFFLU33
✅ Preview: https://markedjs-git-fork-uzitech-fix-node-v12-markedjs.vercel.app

calculuschild · 2021-06-16T05:01:02Z

Aww no... my nice clean optional chaining...

This looks fine. But any chance we can solve the problem by making use of Babel and emitting the es5 version instead of muddying up our source code? That's what Babel is for, right?

zzhjerry · 2021-06-16T05:21:43Z

Aww no... my nice clean optional chaining...

This looks fine. But any chance we can solve the problem by making use of Babel and emitting the es5 version instead of muddying up our source code? That's what Babel is for, right?

Refactoring with babel could come as a separate PR but this fix should be deployed asap as lots of project deployments are broken because of it.

calculuschild · 2021-06-16T05:27:50Z

Refactoring with babel could come as a separate PR

We already generate a transpiled version of Marked with Babel that works with older versions of node. All we would need to do is change one line of code to emit that version by default.

zzhjerry · 2021-06-16T05:34:35Z

Refactoring with babel could come as a separate PR

We already generate a transpiled version of Marked with Babel that works with older versions of node. All we would need to do is change one line of code to emit that version by default.

That's fine but v2.0.10 has become a broken change compare to v2.0.7. Could we at least make sure test passes before releasing the package?

ZarcoNontol

👍

## [2.1.1](v2.1.0...v2.1.1) (2021-06-16) ### Bug Fixes * fix node v12 ([#2109](#2109)) ([af14068](af14068))

github-actions · 2021-06-16T13:50:26Z

🎉 This PR is included in version 2.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

alasdairhurst · 2021-06-16T13:52:08Z

jsdoc/jsdoc#1926

Note - Marked 2.x supported node 8 from the start. jsdoc 3.x depends on marked@^2.x so will pick up any (breaking) node.js support changes that marked introduces. even if things get working on Node.js 12 with this fix, 2.x is still "broken" since jsdoc expects support for node 8.15. Right now there's nothing it can do except roll back and pin marked to a version that did support node 8 or inherit breaking changes whenever marked wants to drop a node version.

alasdairhurst · 2021-06-16T13:30:09Z

package.json

@@ -83,6 +83,6 @@
    "preversion": "npm run build && (git diff --quiet || git commit -am build)"
  },
  "engines": {
-    "node": ">= 8.16.2"
+    "node": ">= 12"


It's one thing to ensure support in CI but it's another thing to change support in a minor version. I understand that only LTS/latest node versions are supported but real functional changes should only happen during a major version, otherwise it's going to break a lot of people.

Sorry, left this a while back but forgot to submit...

If jsdoc wants to support node v8 they could import the es5 version of marked

const marked = require('marked/lib/marked.js');

That should support node v4.

This is why the engines field is misleading and I suggested removing it altogether in #1938 (review)

For the record according to jsdoc engines field it only supports >=v14.17.1

That's good to know that there's an alternative (i missed seeing it in the docs), but the point still kinda stands about the default import potentially breaking over time.

FYI the engines field you linked is on master (jsdoc 4.x prerelease), the current 3.x version is >=8.15.0)

This change is breaking our CI build now. We use this through Typedoc, with that and our lib using Node >= 10.18.0. It would be great if you could revert this change. This is a breaking change as per semantic versioning. I'd recommend you do this in a v3.x release and have a deprecation period to allow dependents to update their packages or find some other mitigation.

error marked@2.1.1: The engine "node" is incompatible with this module. Expected version ">= 12". Got "10.18.0" error Found incompatible module.

@Fleker Node.js 10 reached end of life in April and is no longer supported https://nodejs.org/en/about/releases/

You can probably disable that error if you really need to with yarn install --ignore-engines

calculuschild · 2021-06-16T20:23:14Z

@UziTech Looks like there are still breaking issues with this (might need to revert). I think we could solve a lot of problems (and open up the use of ES6 or even up to the latest ES2020 features in the source code) if we instead just default to the es5 version.

styfle · 2021-06-16T21:32:40Z

@calculuschild The issues is not ES6 (aka ES2015). The issue is the use of optional chaining, which was introduced in ES2020.

See the features and versions here: https://github.com/tc39/proposals/blob/master/finished-proposals.md

This PR solves the root cause: we need to be testing in Node 12 if we intend to support it (thanks @UziTech for also updated engines 🙂)

In the future, we need to drop support (or rather introduce new ES features) under a major semver.

calculuschild · 2021-06-16T21:36:26Z

The issue is the use of optional chaining, which was introduced in ES2020.

@styfle You are correct, I meant to say ES2020. But the solution remains: if we supply the es5 version as the default then we can still use optional chaining, whatever ES2020/ ES2015 feature we want in the source, since Babel nicely converts it all to a version that is compatible with older Node versions. It used to be that the ES6 version of Marked.js ran slightly faster, but that is also no longer the case; I see no reason not to supply the Babelified ES5 version to users. Using optional chaining is not a breaking feature needing a major semver change if it gets transpiled out anyway. This would also allow us to roll back the Node version change in this PR so that we aren't dropping support outside of a major semver change.

That is the entire purpose of the existence of Babel: to make newer ES features backwards-compatible.

All we need to do, from my understanding is change this line:

marked/package.json

Line 6 in da1c57f

"main": "./src/marked.js",

to this:

"main": "./lib/marked.js",

UziTech · 2021-06-17T00:01:11Z

@calculuschild we should also change the ci to testing the built version. I don't know how that will affect PRs or stack traces. Maybe we need to add sourcemaps as well? I feel like this small change isn't as small as first thought.

I'm all for defaulting to es5 version, especially if it is faster, but I think we will have to think through all of the implications.

calculuschild · 2021-06-17T00:13:09Z

Good points. I believe the rollup-plugin-babel package that we already use has an option to generate sourceMaps which should be just an option to set.

As for getting into the ci I wouldn't know where to start for this project.

fix: fix node v12

7abbaa4

vercel bot deployed to Preview June 16, 2021 04:55 View deployment

UziTech marked this pull request as ready for review June 16, 2021 04:56

UziTech requested review from calculuschild, davisjam, joshbruce and styfle June 16, 2021 04:56

calculuschild approved these changes Jun 16, 2021

View reviewed changes

kkaulen mentioned this pull request Jun 16, 2021

Error: SyntaxError: Unexpected token '.' semantic-release/semantic-release#1981

Closed

jonaslagoni mentioned this pull request Jun 16, 2021

Unable to generate : prevRenderer issue asyncapi/html-template#214

Closed

ZarcoNontol approved these changes Jun 16, 2021

View reviewed changes

UziTech merged commit af14068 into markedjs:master Jun 16, 2021

github-actions bot pushed a commit that referenced this pull request Jun 16, 2021

chore(release): 2.1.1 [skip ci]

a2af658

## [2.1.1](v2.1.0...v2.1.1) (2021-06-16) ### Bug Fixes * fix node v12 ([#2109](#2109)) ([af14068](af14068))

github-actions bot added the released label Jun 16, 2021

alexbakum mentioned this pull request Jun 16, 2021

Upgraded marked lib dependency at the JSDoc 3.6.7 release is broken on nodejs <= 14 jsdoc/jsdoc#1926

Closed

alasdairhurst reviewed Jun 16, 2021

View reviewed changes

stevenjoezhang mentioned this pull request Jun 16, 2021

My hexo has some wrong hexojs/hexo#4719

Closed

dpinol mentioned this pull request Jun 17, 2021

fix(contentful): marked 2.1.0 was not compatible with node 12 hubtype/botonic#1643

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix node v12 #2109

fix: fix node v12 #2109

UziTech commented Jun 16, 2021 •

edited

Loading

vercel bot commented Jun 16, 2021 •

edited

Loading

calculuschild commented Jun 16, 2021 •

edited

Loading

zzhjerry commented Jun 16, 2021

calculuschild commented Jun 16, 2021

zzhjerry commented Jun 16, 2021 •

edited

Loading

ZarcoNontol left a comment

github-actions bot commented Jun 16, 2021

alasdairhurst commented Jun 16, 2021 •

edited

Loading

alasdairhurst Jun 16, 2021

alasdairhurst Jun 16, 2021

UziTech Jun 16, 2021 •

edited

Loading

UziTech Jun 16, 2021

alasdairhurst Jun 16, 2021 •

edited

Loading

Fleker Jun 16, 2021

styfle Jun 16, 2021

calculuschild commented Jun 16, 2021 •

edited

Loading

styfle commented Jun 16, 2021

calculuschild commented Jun 16, 2021 •

edited

Loading

UziTech commented Jun 17, 2021 •

edited

Loading

calculuschild commented Jun 17, 2021 •

edited

Loading

fix: fix node v12 #2109

fix: fix node v12 #2109

Conversation

UziTech commented Jun 16, 2021 • edited Loading

Description

Contributor

Committer

vercel bot commented Jun 16, 2021 • edited Loading

calculuschild commented Jun 16, 2021 • edited Loading

zzhjerry commented Jun 16, 2021

calculuschild commented Jun 16, 2021

zzhjerry commented Jun 16, 2021 • edited Loading

ZarcoNontol left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 16, 2021

alasdairhurst commented Jun 16, 2021 • edited Loading

alasdairhurst Jun 16, 2021

Choose a reason for hiding this comment

alasdairhurst Jun 16, 2021

Choose a reason for hiding this comment

UziTech Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

UziTech Jun 16, 2021

Choose a reason for hiding this comment

alasdairhurst Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Fleker Jun 16, 2021

Choose a reason for hiding this comment

styfle Jun 16, 2021

Choose a reason for hiding this comment

calculuschild commented Jun 16, 2021 • edited Loading

styfle commented Jun 16, 2021

calculuschild commented Jun 16, 2021 • edited Loading

UziTech commented Jun 17, 2021 • edited Loading

calculuschild commented Jun 17, 2021 • edited Loading

UziTech commented Jun 16, 2021 •

edited

Loading

vercel bot commented Jun 16, 2021 •

edited

Loading

calculuschild commented Jun 16, 2021 •

edited

Loading

zzhjerry commented Jun 16, 2021 •

edited

Loading

alasdairhurst commented Jun 16, 2021 •

edited

Loading

UziTech Jun 16, 2021 •

edited

Loading

alasdairhurst Jun 16, 2021 •

edited

Loading

calculuschild commented Jun 16, 2021 •

edited

Loading

calculuschild commented Jun 16, 2021 •

edited

Loading

UziTech commented Jun 17, 2021 •

edited

Loading

calculuschild commented Jun 17, 2021 •

edited

Loading