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

Feedback #9

Closed
developit opened this issue Aug 31, 2022 · 1 comment · Fixed by #10
Closed

Feedback #9

developit opened this issue Aug 31, 2022 · 1 comment · Fixed by #10

Comments

@developit
Copy link

Some feedback from my initial read-through:

Output to esm, cjs, and umd formats

It might be helpful to use an example when explaining that cjs is sometimes worth catering to separately from umd. I'd suggest using the example of conditional require() based on process.env.NODE_ENV, a common technique within the React ecosystem (because React itself uses this technique). These don't generally produce conditional dependencies when bundled from umd (conditions get ignored and dependencies included unconditionally).

Don't minify

I'm not quite sure I agree with this section as-written. Application bundlers like Webpack do a significantly poorer job of minifying library code, and can't be configured to minify a given library in the way that would be most optimal. As an example, constant inlining is extremely valuable for performance and size when compiling (minifying) a library, but constant inlining within dependencies is only currently possible in Rollup with specific configuration.

Perhaps it would be worth breaking apart the minification advice. It's generally the case that application bundling+minifying setups do a reasonably effective job of minifying whitespace and local names, so libraries can skip those two forms of minification and produce a package containing relatively readable code. However, application bundling+minifying setups are generally much less effective at optimizing across module boundaries, and cannot make assumptions about code structure that are required to minify optimally.

I've generally tried to explain this as: if you're concerned about size and perf, you can compile your code before publishing using a tool like Terser, but configure it to perform optimizations rather than whitespace and name compression.

As an illustrative example: none of the current commonly-used minifiers will compress property names in the code from an npm module. Compressing/inlining/dissolving properties of internal objects within a library is a hugely valuable optimization for both size and performance. Packages that publish code without running optimizations like this are leaving these benefits on the floor.

Target modern browsers

It might be worth including a quick explanation of legacy browser support fallback in the TS example:

As one example, if you're transpiling from TypeScript, you could create two versions of your package's code:

  1. an esm version with modern JavaScript generated by setting "target" in your tsconfig.json to ESNext
  2. a umd version with more broadly-compatible JavaScript generated by setting "target" in your tsconfig.json to ES5

Most users will get the modern code, but those using older bundler configurations or loading the code using a <script> tag will get the version with additional transpilation for older browser support.

Define your exports

  • nit: "export maps" was used super early-on, but quickly replaced with "package exports" - I think much of the documentation now refers to them as such.
  • "script" isn't used by anything, as far as I am aware of. I'm not sure why the Webpack docs mention it, webpack doesn't seem to ever use the property.
  • "module" - it might be worth mentioning that this field's purpose is an optimization for systems that transparently handle CommonJS require() of an ES Module, so that require('x') and import 'x' bundle/execute the same ES Module instead of resulting in two copies.
  • "default" - there is generally no reason to specify both "require" and "default", since there are no module systems that resolve Package Exports but lack support for both CommonJS and ESM.
  • It might be worth using more cautious language in the bit about "development" + "production". Because these aren't supported by default in Rollup or Node, relying on them for nontrivial differentiation between dev/prod bundles is uncommon and a bit of a support/onboarding burden.

Set the default module type for your JS files

It's a tiny detail, but I believe the existence of a package.json within a subdirectory currently will not override the default .js file extension association if that file is reached by resolving from the "exports" field of a package.json in a parent directory. I might be wrong on this though!

Set the browser field

Not sure if it's worth mentioning, but unpkg.com currently ignores the "browser" field. You can set an "unpkg" field to control which path unpkg will serve when a package is requested directly (unpkg.com/foo). A lot of folks include both browser and unpkg, both pointing to a UMD bundle.

@frehner
Copy link
Owner

frehner commented Sep 1, 2022

Not sure if it's worth mentioning, but unpkg.com currently ignores the "browser" field.

It seems like something updated, and now it supports the browser field. At least, trying out

https://unpkg.com/@shopify/hydrogen-ui-alpha

Correctly redirects to

https://unpkg.com/@shopify/hydrogen-ui-alpha@2022.7.2/dist/umd/hydrogen-ui.prod.js

Without any unkpg field in the package.json for that package. But I'll keep the corresponding PR text in there still.

frehner added a commit that referenced this issue Sep 1, 2022
* Update sections and wording based on #9

* Add vite to the list of bundlers that understand dev/prod

* Refer to bundlers in general

* Slight text update on unpkg

* Self review

* Update text on minification to be more generic

* Hedge a little more, because hedging is fun

* Fix typo and wording for minifier text
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.

2 participants