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

fix: update min node to v18 #2767

Merged
merged 1 commit into from
May 2, 2023
Merged

fix: update min node to v18 #2767

merged 1 commit into from
May 2, 2023

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Mar 30, 2023

🚨 BREAKING CHANGE 🚨

do not submit until we are ready to release v5.0.0

Description

Update minimum node version to v18

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.

@vercel
Copy link

vercel bot commented Mar 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
marked-website ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 30, 2023 at 6:20AM (UTC)

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets also include 16 since its still in maintenance until September

https://github.com/nodejs/Release/blob/main/README.md

@UziTech
Copy link
Member Author

UziTech commented Mar 30, 2023

Since v16 is out of active support I think it would be better to skip supporting it rather than require another major release in September. We can wait for v20 to come out before merging this.

https://nodejs.org/en/blog/announcements/nodejs16-eol

@styfle
Copy link
Member

styfle commented Mar 31, 2023

Are there other upcoming breaking changes we can batch this in with?

Also note the PR title should change feat: to BREAKING CHANGE:

@UziTech
Copy link
Member Author

UziTech commented Mar 31, 2023

Ya there is #2766. I would also like to fix #1745 adding a param to html renderer that tells it if it is block html or inline html.

@UziTech UziTech changed the title fix: update min node to v18 BREAKING CHANGE: update min node to v18 Mar 31, 2023
@calculuschild
Copy link
Contributor

calculuschild commented Mar 31, 2023

Any other options we can extract into extensions (perhaps by filling out Hooks?)

@UziTech
Copy link
Member Author

UziTech commented Mar 31, 2023

Ya there are some easy ones that can be moved to extensions without adding hooks, like baseUrl, langPrefix, and mangle.

headerIds and headerPrefix already have the marked-gfm-heading-id extension.

Although with headerId being true by default it may affect a lot more users if we deprecate it.

@UziTech
Copy link
Member Author

UziTech commented Apr 22, 2023

Now that node V20 is out I think we should merge these breaking changes (#2766, #2767, and #2768) and release v5.

@UziTech UziTech changed the title BREAKING CHANGE: update min node to v18 fix: update min node to v18 May 2, 2023
@UziTech UziTech merged commit c6852f5 into markedjs:master May 2, 2023
@UziTech UziTech deleted the update-node branch May 2, 2023 04:27
github-actions bot pushed a commit that referenced this pull request May 2, 2023
# [5.0.0](v4.3.0...v5.0.0) (2023-05-02)

### Bug Fixes

* deprecate options ([#2766](#2766)) ([62d3312](62d3312))
* update min node to v18 ([#2767](#2767)) ([c6852f5](c6852f5))

### Features

* add block param to html renderer ([#2768](#2768)) ([fa21b9f](fa21b9f))

### BREAKING CHANGES

* deprecate options
* minimum supported node version v18
@benmccann
Copy link
Contributor

Is there any code in marked that actually requires Node 18? We use marked in our framework and while we use Node 18 in our own projects, we couldn't drop Node 16 support as it's a breaking change and we only infrequently release new majors. I'm curious if there's a pressing reason to drop Node 16 and if it might be able to be added back?

@UziTech
Copy link
Member Author

UziTech commented May 2, 2023

The only code that I know of that requires node 18 is in our build system. Marked should work with node 12 still. This minimum node requirement just means we only test on node v18+

@UziTech
Copy link
Member Author

UziTech commented May 2, 2023

If you see something that doesn't work it would be ok to submit a PR to make it work with node 16. We just can't guarantee it will continue to work with node 16

@benmccann
Copy link
Contributor

Thanks. The engines field in package.json prevents it from being installed. Would it be okay if I send a PR to either lower the value in that field or remove that field?

@UziTech
Copy link
Member Author

UziTech commented May 2, 2023

No because we wouldn't be able to verify every change works with node 16. I think if you still need node v16 support it would be best for you to continue using marked v4. Node 16 eol is later this year. It should be fine for you to wait to update to marked v5 and drop node v16 support to after that.

@benmccann
Copy link
Contributor

Yeah, we'll probably stick with marked 4. I'm curious what in the build process requires Node 18. If it's easy enough maybe I could make it support Node 16 as well

@UziTech
Copy link
Member Author

UziTech commented May 3, 2023

Semantic release requires node v18

@yoyo837
Copy link

yoyo837 commented May 4, 2023

The minimum version requirement of node 18 is too aggressive.

@benmccann
Copy link
Contributor

I sent a PR to lower the version in the engines field: #2886

Semantic release requires node v18

The release job only runs on the latest lts version of Node, so that won't block supporting older versions. We can still test marked against older versions regardless of what versions sematic release supports

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.

5 participants