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: Convert to ESM #2227

Merged
merged 11 commits into from Nov 2, 2021
Merged

fix: Convert to ESM #2227

merged 11 commits into from Nov 2, 2021

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Oct 7, 2021

Marked version: master

Markdown flavor: all

Description

Converts the source code to ESM to be more modern and allow Rollup to better handle it. Rollup will still convert the final code to be UMD / ESM, but will do so more efficiently resulting in smaller output files

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 Oct 7, 2021

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/4GVhinkD5B8e9yGBnM2czZnEa748
✅ Preview: https://markedjs-git-fork-benmccann-esm-named-only-markedjs.vercel.app

test/helpers/helpers.js Outdated Show resolved Hide resolved
test/unit/marked-spec.js Outdated Show resolved Hide resolved
@UziTech UziTech mentioned this pull request Oct 8, 2021
5 tasks
@UziTech
Copy link
Member

UziTech commented Oct 8, 2021

We could probably remove lib/marked.esm.js with this update. Or make it just a file that exports src/marked.js

@benmccann
Copy link
Contributor Author

Ok, I've added "type": "module" and converted all the tests to ESM as well

We could probably remove lib/marked.esm.js with this update. Or make it just a file that exports src/marked.js

We could, but I actually think it might be preferable to keep as is so that we know the UMD and ESM versions have the same exposed APIs. I think it could be easier to document and explain that way.

lib/marked.js Show resolved Hide resolved
@benmccann
Copy link
Contributor Author

@UziTech I was wondering if you had any other thoughts about this? Is it something we could move forward with in the near future to avoid merge conflicts from accruing?

@UziTech
Copy link
Member

UziTech commented Oct 20, 2021

Looks like vercel is failing because npm run build:docs is failing. That will need to be fixed before this is merged.

@benmccann
Copy link
Contributor Author

Ok, thanks for letting me know. I didn't have permission to see the Vercel link, so didn't know what was wrong. I've updated it so that it's working now

@UziTech
Copy link
Member

UziTech commented Oct 22, 2021

I'll take a look at this soon and make sure all the ways of importing marked works or see where we would have to update our docs.

@UziTech
Copy link
Member

UziTech commented Oct 24, 2021

Can you fix the npm run bench command? it seems to have some issues with ESM.

Copy link
Contributor

@calculuschild calculuschild left a comment

Choose a reason for hiding this comment

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

Seems fine except for a bunch of random whitespace being be added between words in the documentation.

I really don't have experience in this side of JavaScript with module packaging and stuff though, so I don't know how useful my review is.

man/marked.1.txt Outdated Show resolved Hide resolved
man/marked.1.txt Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor Author

I think that might have been from @UziTech 's changes. I'm not sure if perhaps that file is autogenerated from something, but I just manually updated it

@UziTech
Copy link
Member

UziTech commented Oct 31, 2021

That is auto generated but we can edit it manually.

marked/Makefile

Lines 12 to 13 in e93f800

man/marked.1.txt:
groff -man -Tascii man/marked.1 | col -b > man/marked.1.txt

@benmccann
Copy link
Contributor Author

Cool. Do we need to wait for @davisjam, @joshbruce and @styfle to weigh in before merging?

@UziTech
Copy link
Member

UziTech commented Nov 1, 2021

I was just giving a little bit of time for anyone to weigh in. I'll merge this later today if there is no issues.

@UziTech UziTech changed the title Convert to ESM fix: Convert to ESM Nov 2, 2021
@UziTech UziTech merged commit 4afb228 into markedjs:master Nov 2, 2021
github-actions bot pushed a commit that referenced this pull request Nov 2, 2021
# [4.0.0](v3.0.8...v4.0.0) (2021-11-02)

### Bug Fixes

* Convert to ESM ([#2227](#2227)) ([4afb228](4afb228))

### BREAKING CHANGES

* - Default export removed. Use `import { marked } from 'marked'` or `const { marked } = require('marked')` instead.
- `/lib/marked.js` removed. Use `/marked.min.js` in script tag instead.
- When using marked in a script tag use `marked.parse(...)` instead of `marked(...)`
@github-actions
Copy link

github-actions bot commented Nov 2, 2021

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

XuluWarrior added a commit to XuluWarrior/jsonresume-theme-kards that referenced this pull request Dec 3, 2021
XuluWarrior added a commit to XuluWarrior/jsonresume-theme-orbit-original that referenced this pull request Dec 3, 2021
XuluWarrior added a commit to XuluWarrior/jsonresume-theme-orbit that referenced this pull request Jan 1, 2022
@jimmywarting
Copy link
Contributor

Ty 4 this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants