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

Add browser field to package.json #1630

Closed
ziir opened this issue Mar 26, 2020 · 15 comments · Fixed by #1661
Closed

Add browser field to package.json #1630

ziir opened this issue Mar 26, 2020 · 15 comments · Fixed by #1661

Comments

@ziir
Copy link

ziir commented Mar 26, 2020

Hi,

First of all, thank you for this great package, we've been using it for years and are happy with it. :)

We recently upgraded one of our app from [email protected] to [email protected] considering it a minor release from scanning the CHANGELOG and everything seemed to work without requiring changes.

It is only (much, sadly) later that we discovered that from [email protected] has multiple cross-browser compatibilities issues:

As we did not have this issue with previous versions of marked and we did not immediately see this sudden change in browser compatibility from the CHANGELOG / README, we took a peek at marked's package.json file.
It appears marked exposes src/marked.js as primary entry-point, which will be used by bundlers - in our case, Webpack.
As a result, Webpack resolved marked's source files and bundled these into our production bundles.

It is our understanding that you wish to keep src/marked.js as the main entrypoint, allowing Node.js users to use the un-bundled version.
However, would you consider adding the browser entry as well to marked's package.json file so it can safely be consumed directly for web targets using a bundler?

The following patch should be sufficient, has been tested for our use-case.

   "description": "A markdown parser built for speed",
   "author": "Christopher Jeffrey",
   "version": "0.8.2",
+  "browser": "./lib/marked.js",
   "main": "./src/marked.js",
   "bin": "./bin/marked",
   "man": "./man/marked.1",

Relevant reading:

Note: I have read #1585 and #1571 and decided to open a new issue nevertheless, since the discussion there seem to have diverted and is not easy to parse. In any case, we'd be happy to provide PRs and participate further in discussion around this topic for marked.

Thank you for your consideration, have a great day.

@styfle
Copy link
Member

styfle commented Mar 26, 2020

Please see #1573

@ziir
Copy link
Author

ziir commented Mar 26, 2020

Thanks for your response, I wasn't aware this package once had a browser field before being removed.

However, it is my understanding that it was removed so bundlers would not favor it over the module field, which was then removed as well following discussion of #1571.

We could use a workaround for our apps such as using the ES5 build though I believe marked consumers would benefit from it having all main, module and browser fields linking to the relevant entrypoints, offering sane defaults while still allowing people to override (using specific imports, bundler options, etc...) should they want to.

Let me know what you think,

@styfle
Copy link
Member

styfle commented Mar 26, 2020

Good point! What are your thoughts @UziTech @Andarist?

@UziTech
Copy link
Member

UziTech commented Mar 27, 2020

I like the idea of using the browser property if it will fix #1585. @Andarist would know more about it than me.

@Andarist
Copy link
Contributor

So the question really is - what are your targets and requirements. For example - 0.8 has dropped support for older versions of node. Was it intentional or accidental? Do you feel that it is OK to only support newer versions of node but you still want to support legacy IE11 out of the box? Is the intention to support IE11 for bundler users or just for UMD users? Also - keep in mind that it's not that IE11 is not supported right now, it just requires transpilation on consumers side rather than in marked itself. By transpiling down to ES5 (what IE11 supports) you are going to increase the bundlesize for everyone, even for those who had dropped IE11 support already.

Don't take me wrong - at my day job I'm still, sadly, required to support IE11. I'm just asking about all of this and outlining some problems/tradeoffs so I can give you educated advice later, based on your requirements.

@UziTech
Copy link
Member

UziTech commented Mar 27, 2020

@Andarist I think we do want to support IE11 for browsers but node LTS for node. Since we already compile marked to ES5 in lib/marked.js I don't think it will be any extra work.

Currently if someone uses webpack it will bundle src/marked.js unless they import marked/lib/marked.js.

@Andarist
Copy link
Contributor

Since we already compile marked to ES5 in lib/marked.js I don't think it will be any extra work.

Right, it will impose some extra bytes on consumers - mainly that pesky UMD wrapper 😉I can optimize the current setup if you are into over-optimizing the bundlesize and you are not afraid of some extra rollup config (I can make most of it shared though! I only really need a single rollup config template to fill in formats and targets).

@styfle
Copy link
Member

styfle commented Mar 29, 2020

I think the part we’re missing is tests for webpack. We assumed that module would work and browser would work but we ran into problems. The best way to prevent these problems is to test the package against different bundlers, perhaps with npm pack in CI.

@ziir
Copy link
Author

ziir commented Apr 2, 2020

I feel tests for bundlers would be a bit overkill but I guess it wouldn't hurt either.

Right, it will impose some extra bytes on consumers

Some extra bytes w/ extended cross-browser compatibility for browser target consumers is like a good trade-off to me :) There surely could be some further otpimizations that could be discussed in another issue maybe?

@UziTech @styfle May I open a PR for the browser field change?

@Andarist
Copy link
Contributor

Andarist commented Apr 2, 2020

The best way to prevent these problems is to test the package against different bunglers, perhaps with npm pack in CI.

The problem isn't caused by the current setup not being compatible with bundlers - it's just that shipped code has es6+ syntax, thus it's not compatible with IE11. So I would say this is a different problem and either way this is just going to be hard to test, just like @ziir said. I'm not sure if it's worth it.

@UziTech @styfle May I open a PR for the browser field change?

I have already started working on this and should open a PR for it today or tomorrow.

@UziTech
Copy link
Member

UziTech commented Apr 13, 2020

@Andarist Where are we on this? If changing the browser field makes bundlers grab the es5 version it would be great to get that change in the next release.

@Andarist
Copy link
Contributor

You can check out my WIP commit here - Andarist@d13f35a

It changes the source code module format from CJS to ESM as it allows for Rollup to generate a smaller bundle. Apart from that - it unifies build configs and introduces 2 new configs: for node and for browser.

It's not finished yet - just some rough edges around vuln-regex.js, maybe some other minor issues. Not sure if you want to proceed with this approach though, so I'm not sure if I should invest more time into this.

An additional benefit (although technically a breaking change) of this approach is that your internal file structure gets hidden, it's no longer possible to require some "deep" path - everything must be required from a single public entrypoint.

If you don't want to proceed with this - just add "browser": "./lib/marked.js" to your package.json

@UziTech
Copy link
Member

UziTech commented Apr 14, 2020

The problem I ran into when converting to ESM is that it was about 20% slower than cjs in node v12. If it becomes faster I would be ok with changing the source to ESM.

@Andarist
Copy link
Contributor

Keep in mind that I do not propose distributing ESM - I'm just proposing to author the source code in ESM so its static nature can be utilized better during compilation time. Everything that is shipped to npm would still be in CJS/UMD format.

@ziir
Copy link
Author

ziir commented Apr 28, 2020

Any news on this? We'd love to be able to update marked and, generally speaking, using it with more confidence.

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 a pull request may close this issue.

4 participants