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

0.8.0 #1571

Merged
merged 1 commit into from
Dec 12, 2019
Merged

0.8.0 #1571

merged 1 commit into from
Dec 12, 2019

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented Dec 4, 2019

Release Notes

Breaking changes

Fixes

Docs

Dev Dependencies


Publisher

  • $ npm version has been run.
  • Release notes in draft GitHub release are up to date
  • Release notes include which flavors and versions of Markdown are supported by this release
  • Committer checklist is complete.
  • Merge PR.
  • Publish GitHub release using master with correct version number.
  • $ npm publish has been run.
  • Create draft GitHub release to prepare next release.

Note: If merges to master occur after submitting this PR and before running $ npm pubish you should be able to

  1. pull from upstream/master (git pull upstream master) into the branch holding this version,
  2. run $ npm run build to regenerate the min file, and
  3. commit and push the updated changes.

Committer

In most cases, this should be someone different than the publisher.

  • Version in package.json has been updated (see PUBLISHING.md).
  • The marked.min.js has been updated; or,
  • release does not change library.
  • CI is green (no forced merge required).

@vercel
Copy link

vercel bot commented Dec 4, 2019

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

🔍 Inspect: https://zeit.co/markedjs/markedjs/7ocppg1jw
🌍 Preview: https://markedjs-git-fork-uzitech-release-080.markedjs.now.sh

@Andarist
Copy link
Contributor

Andarist commented Dec 4, 2019

While this is going to be a breaking change - maybe you would like to consider changing a few more things? I would like to propose changing exported shape for better tree-shakeability. Mainly I don't believe using a default export is a good fit here (especially the way it is used right now) and also I would like to propose to skip using static properties on functions - but this one would have to be evaluated first because by a quick glance at the source code I'm not sure why they are used in the first place (maybe they are for some reason needed, not sure)

I'm of course offering my help in refactoring and I'm open to discussing this more.

@UziTech
Copy link
Member Author

UziTech commented Dec 4, 2019

@Andarist if you want to create a PR with your proposed changes we could squeeze it in here if we think it is necessary.

@Andarist
Copy link
Contributor

Andarist commented Dec 4, 2019

@UziTech Ok, I'm going to prepare something later tonight. Stay tuned!

@UziTech UziTech requested review from davisjam and joshbruce December 6, 2019 17:26
@Andarist
Copy link
Contributor

Andarist commented Dec 7, 2019

Keep in mind that your main & module entries (cjs & esm) have incompatible shapes right now. module.exports = value is not an equivalent to export default value. This can break your consumers in a situation as follows:

// webpack consumer
require('./foo')
// foo.js
var localMarked = require('marked') // webpack will expose `{ default: marked }` as `localMarked`
localMarked('**md**') // error!

This example maybe doesn't illustrate it best - but to describe it in more details: webpack will see that "module" field and will load it instead of "main", but this "module" exposes different shape than what was expected by foo.js when using require.

@UziTech
Copy link
Member Author

UziTech commented Dec 8, 2019

This seems to be a known issue with webpack webpack/webpack#4742

I have found the following solutions:

  1. Leave it as is since this is a known issue with webpack and document the solution.
  2. remove module field in package.json
  3. Create named export marked
    This would allow anyone to use
    const { marked } = require('marked');
    We would still keep the default export for anyone who wants to use it.

I prefer #1 so when webpack fixes this we won't need to change anything.

@styfle
Copy link
Member

styfle commented Dec 12, 2019

I don't think webpack is going to change, the issue is over 2 years old. I also think the maintainer is correct, packages should not export different things from module and main.

What are the implications for number 2: remove module field from package.json? That seems to be the best solution.

@web-padawan
Copy link

What are the implications for number 2: remove module field from package.json? That seems to be the best solution.

Some existing tools rely on that field as indicator of the "modern" packages shipping ES modules.

See for example @pika/web https://github.com/pikapkg/web and all the tools by @pikapkg

@UziTech
Copy link
Member Author

UziTech commented Dec 12, 2019

@styfle I believe #2 would prevent code splitting/tree shaking although I'm not sure if that is actually an issue with a library like marked that only has one export and pretty much the entire codebase is used for a single purpose.

Without the module field anyone wanting to use the esm version in node would need to use import marked from 'marked/lib/marked.esm.js';

We could move the esm build to /esm.mjs so they could use import marked from 'marked/esm';

@UziTech
Copy link
Member Author

UziTech commented Dec 12, 2019

I think removing the module field would be the easiest solution. It would make marked backwards compatible and we can always fix code splitting/tree shaking in a later version if we need to.

@styfle
Copy link
Member

styfle commented Dec 12, 2019

Without the module field anyone wanting to use the esm version in node

I don't think Node supports the module field.

Perhaps you are thinking of the "type": "module" property?

@Andarist
Copy link
Contributor

Yes - if you dont want to change CJS export shape right now then I would also recommend dropping the "module" field right now. I would love "module" being added - as the community, I believe we should move towards ESM as much as we can as it's the part of the language - but only if it's done right (so without any drawbacks).

At the moment marked is not tree-shakeable anyway (but it could! if you'd be willing to change exports shape 😉 ).

@UziTech UziTech mentioned this pull request Dec 12, 2019
4 tasks
@UziTech
Copy link
Member Author

UziTech commented Dec 12, 2019

I think this is ready to publish once we get one more approval from @joshbruce or @davisjam

@UziTech UziTech merged commit 416003b into markedjs:master Dec 12, 2019
@UziTech UziTech deleted the release-0.8.0 branch December 12, 2019 20:47
@UziTech
Copy link
Member Author

UziTech commented Dec 12, 2019

v0.8.0 has been released 🎉

@Andarist Thanks for your PRs and the help with this release.

@Andarist
Copy link
Contributor

No problem. If you'd like to discuss tree-shakeability and changing exports shape in the future I would gladly help you out on this one. Configuring things "right" for bundlers and other possible consumers is surprisingly hard and I have good expertise on that matter.

@styfle
Copy link
Member

styfle commented Dec 13, 2019

Thanks @Andarist 👍

A related issue has been opened in pikapkg that you might want to engage in.

FredKSchott/analyze-npm#9

@UziTech UziTech mentioned this pull request Dec 18, 2019
@UziTech UziTech mentioned this pull request Apr 20, 2021
5 tasks
@UziTech UziTech mentioned this pull request Aug 2, 2021
5 tasks
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
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