justmoon / node-extend

Simple function to extend objects
MIT License
341 stars 68 forks source link

feat: distribute an ESM version #57

Closed raulfdm closed 3 years ago

raulfdm commented 3 years ago

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.

https://github.com/unifiedjs/unified/issues/153

Summary

Resource

raulfdm commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@ljharb what do you think? https://github.com/justmoon/node-extend/pull/58

ljharb commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

ChristianMurphy commented 3 years ago

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 commented 3 years ago

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: https://github.com/vitejs/vite/issues/3024 and there is a work around available today without needing to change packages: https://github.com/vitejs/vite/issues/3024#issuecomment-860018367

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 commented 3 years ago

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.