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

feat: distribute an ESM version #57

Closed
wants to merge 4 commits into from
Closed

feat: distribute an ESM version #57

wants to merge 4 commits into from

Conversation

raulfdm
Copy link

@raulfdm raulfdm commented Jul 21, 2021

Hello there.

Just open this PR to also distribute an ESM version of extend so it can be used by vite and snowpack.

You can follow this discussion to see more details.

unifiedjs/unified#153

Summary

  • Rename index.js to index.cjs
    • change package.json.main reference
  • Create a index.mjs version from index.js
    • Remove unnecessary 'use strict';
    • replace module.exports with export
    • add it into package.json.module prop

Resource

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

a) this creates two sources of truth for the library - there's nothing in this PR that enforces they're in sync, and it condemns all future changes to this library to be made twice.
b) if vite and snowpack can't consume CJS directly, they will literally never be useful in a professional setting, because the vast majority of the world's JS is not and will never be distributed in ESM.
c) the ESM file is not being tested at all in this PR.
d) Any browser that understands ESM has Object.assign and object spread syntax, which covers almost every use case of this library.

@@ -3,7 +3,8 @@
"author": "Stefan Thomas <justmoon@members.fsf.org> (http://www.justmoon.net)",
"version": "3.0.2",
"description": "Port of jQuery.extend for node.js and the browser",
"main": "index",
"main": "index.cjs",
"module": "index.mjs",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not a standard field, and should not exist in any package. the proper way to distribute ESM is with the "exports" field, and adding that is a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

I was reading this https://2ality.com/2019/10/hybrid-npm-packages.html from Dr. Axel... what do you think to add moth module and export to have a bit of backwards compatibility with rollup, webpack, etc? 🤔

@raulfdm
Copy link
Author

raulfdm commented Jul 21, 2021

a) this creates two sources of truth for the library - there's nothing in this PR that enforces they're in sync, and it condemns all future changes to this library to be made twice.

Unless I add a build tooling like rollup, I don't see a way of doing that. 🤔

If we're open for this option, I can easily add it.

b) if vite and snowpack can't consume CJS directly, they will literally never be useful in a professional setting, because the vast majority of the world's JS is not and will never be distributed in ESM.

That's a debate I cannot afford.

For me, It seems we're trying to moving towards the ESM standard and (IMO) we as community should help as much as we can to help this out.

c) the ESM file is not being tested at all in this PR.

Valid point, I'll add unit tests for that

d) Any browser that understands ESM has Object.assign and object spread syntax, which covers almost every use case of this library.

You're right... @ChristianMurphy Maybe unified doesn't need extend at all? 🤔

@raulfdm raulfdm requested a review from ljharb July 21, 2021 06:15
@raulfdm
Copy link
Author

raulfdm commented Jul 21, 2021

Thanks @ljharb for reviewing that.

I think my only 2 big questions here would be the package.json exports and how we can do a single source of truth.

As I said, the only way I can see that working is by just adding a very minimal rollup config but maybe you have another suggestion

@ljharb
Copy link
Collaborator

ljharb commented Jul 21, 2021

I definitely wouldn't want to use rollup; it tends to violate the spec in a number of ways.

imo the best help the community can provide is pushing back against attempts to deny the existence of the largest repository of code in any language on the entire planet being in CJS format.

@raulfdm
Copy link
Author

raulfdm commented Jul 21, 2021

@ljharb what do you think? #58

@ljharb
Copy link
Collaborator

ljharb commented Jul 21, 2021

esbuild is a very, very young tool, and I'm still not clear on the benefits. node itself can import CJS, and any tool that can't do that is imo a flawed one.

@raulfdm
Copy link
Author

raulfdm commented Jul 21, 2021

imo the best help the community can provide is pushing back against attempts to deny the existence of the largest repository of code in any language on the entire planet being in CJS format.

I hear you.. but I think this train has already left :/

As I said before, this is a debate I can't afford even though I've read a bunch of explanation we're moving away from CJS (which isn't ES standard even though it's consolidate with node packages).

@ljharb
Copy link
Collaborator

ljharb commented Jul 21, 2021

It may have left for some, but in no way is it a foregone conclusion, or even a significant percentage of npm packages that have made this kind of change. The tooling needs to adapt, not the ecosystem.

To be concrete, I maintain over 300 packages, and this question has come up maybe on one other package before this PR.

@raulfdm
Copy link
Author

raulfdm commented Jul 21, 2021

esbuild is a very, very young tool, and I'm still not clear on the benefits. node itself can import CJS, and any tool that can't do that is imo a flawed one.

Look, esbuild was only used to transpile the index.js to index.mjs and then having a single source of truth as you've pointed.

We're talking about having an hybrid package which uses 2 different ways of export modules.

If you don't want to use rollup, esbuild, webpack seems an overkill, maybe I'd give a step back and ask if you're open for supporting this or not.

It's totally fine if you don't want to, unless you said this explicit and I can bring that point to unified maintainers

@ljharb
Copy link
Collaborator

ljharb commented Jul 21, 2021

I'm still trying to understand why it's necessary, and why anyone would be trying to use vite or snowpack if the only packages they can use with it are ones that ship a native ESM build, that depends on only packages with a native ESM build.

@raulfdm
Copy link
Author

raulfdm commented Jul 21, 2021

Unfortunately I cannot answer this question, since I'm not involved on vite's architecture discussions 😕

@ChristianMurphy
Copy link

You're right... @ChristianMurphy Maybe unified doesn't need extend at all?

In most cases unified directly uses Object.assign, it uses extend specifically for deep merging: https://github.com/unifiedjs/unified/blob/2323d38e2c74708f5a453ebfea42f80e0454a435/lib/index.js#L84

@ChristianMurphy
Copy link

why anyone would be trying to use vite or snowpack if the only packages they can use with it are ones that ship a native ESM build, that depends on only packages with a native ESM build.

This appears to be a bug which the vite team considers a priority: vitejs/vite#3024
and there is a work around available today without needing to change packages: vitejs/vite#3024 (comment)

I think snowpack already supports mixed CJS and ESM https://www.snowpack.dev/concepts/how-snowpack-works#using-npm-dependencies but I could be wrong.

@ljharb
Copy link
Collaborator

ljharb commented Jul 21, 2021

Given that vite and snowpack should thus be able to handle CJS dependencies perfectly fine (without which they'd be useless), there doesn't seem to be any need to make changes in this package.

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.

3 participants