-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Update docs to clarify Node.js and browser support #1938
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/markedjs/markedjs/8wzuVhmbYaeYkB23Zj8TWXY7CgTq |
|
||
**Node.js:** Only [current and LTS](https://nodejs.org/en/about/releases/) Node.js versions are supported. End of life Node.js versions may become incompatible with Marked at any point in time. | ||
|
||
**Browser:** Not IE11 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@UziTech Makes sense to also document the supported browsers but I can't seem to find them after a bit of light digging. Rollup doesn't seem to be using any specific browserlist for transpiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it compiles to es5 but some functions (like startsWith
) might need polyfills for browsers that don't include them. Hence the note about IE11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just link to package.json
so that can be the canonical place to determine which Node.js versions are supported.
We should also add this to the docs website, probably after the "Supported Markdown specifications" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth to a set a rule which browsers are supported and to enforce it using https://www.npmjs.com/package/eslint-plugin-compat
I noticed the version in component.json was out of sync. Not sure what this is used for or how it should be updated, but i'm guessing this should be matching the package.json version? |
I'm not even sure what that is for. Looks like it was for something a long time ago. It should probably just be deleted. |
Had a little dig a bit further and it's this old tool |
Is this something we can address as part of #2157 before the V3 update? This PR has been open for quite a long time, and seems like this would be the perfect time to either make this change or officially close it out since it relates to semver issues. |
We did update |
Right. Just saying since we did change |
Yes @alasdairhurst if you would rebase this we could update the docs. |
ab92e54
to
4ba3f6f
Compare
@UziTech @calculuschild Rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sticking with it! 💯
@@ -10,6 +10,6 @@ Marked uses [semantic-release](https://github.com/semantic-release/semantic-rele | |||
|
|||
We follow [semantic versioning](https://semver.org) where the following sequence is true `[major].[minor].[patch]`: | |||
|
|||
1. **Major:** There is at least one change to the public API or a break from the [CommonMark](https://spec.commonmark.org/current/) or [GFM](https://github.github.com/gfm/) spec. | |||
1. **Major:** There is at least one change to the public API or a break from the [CommonMark](https://spec.commonmark.org/current/) or [GFM](https://github.github.com/gfm/) spec. Only [current and LTS](https://nodejs.org/en/about/releases/) Node.js versions are supported at any point in time. A drop in support for a Node.js version may not result in a semver major bump to Marked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we plan to do this.
We should do semver major when dropping a version of Node.js and bump the engines field
Line 87 in e781b18
"node": ">= 12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if that happened but I wasn't able to convince everyone on the team :)
I may have missed something though. Not been paying too much attention recently and noticed the 3.0.0 release with the documented breaking change dropping node 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@styfle Currently we test Node v12, Node lts (v14), and Node latest (v16). When Node LTS changes to v16 we will be testing Node v12, Node lts (v16), and Node latest (v17), so Node v14 will no longer be tested. If we had a team of people working on this every day I think we could task someone with updating that, but realistically there are three-ish people working on marked, whenever we have extra time.
I think it is good to set the expectations low. Ideally we won't break Node versions that aren't tested and we we will only update the engines field on semver major releases but I still like saying, realistically, we could break older node versions in patches since only lts and latest are guaranteed to be tested and the best fix might be for you to update Node.
🎉 This PR is included in version 3.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Marked version: 2.0.0
Markdown flavor: n/a
Description
Updates docs to describe Node.js support clearly, and clarify the strategy for dropping old versions of Node.js in relation to semver.
Expectation
More clear docs for consumers
Result
n/a
What was attempted
n/a
Contributor
Committer
In most cases, this should be a different person than the contributor.