TehShrike / deepmerge

A library for deep (recursive) merging of Javascript objects
MIT License
2.75k stars 216 forks source link

Help using deepmerge as module #137

Closed garyo closed 5 years ago

garyo commented 5 years ago

I'm using webpack in a vue.js project. I'm doing import * as deepmerge from 'deepmerge' as recommended in the README, but then I can't actually use it: in my unit tests I get TypeError: deepmerge is not a function when I call deepmerge(foo, bar) or deepmerge.deepmerge(foo, bar). If I print JSON.stringify(deepmerge) I get this:

 { all: [Function: deepmergeAll],
        default: { [Function: deepmerge] all: [Function: deepmergeAll] } }

I've tried deepmerge(foo, bar), deepmerge.deepmerge(foo, bar) and any other combo I can think of but can't make it work. Any hints?

TehShrike commented 5 years ago

What version of deepmerge? (run npm ls deepmerge)

What bundler are you using? (I'm going to guess Webpack)

You'll need deepmerge@3 to get the version that doesn't trigger the Webpack bug.

garyo commented 5 years ago

deepmerge@3.1.0 bundler: webpack (although I notice that @vue/cli-service depends on webpack-chain which is also using deepmerge@1.5.2, but that shouldn't affect my top-level deepmerge)

foo@0.1.0 /mnt/c/dss/foo
├─┬ @vue/cli-service@3.4.0
│ └─┬ webpack-chain@4.12.1
│   └── deepmerge@1.5.2
└── deepmerge@3.1.0
TehShrike commented 5 years ago

What's running your unit tests?

Everything should work with import * as deepmerge.

deepmerge@3.1.0 is a UMD package, so whatever bundler thingy is turning that into an object with a default property is screwing up pretty badly.

In order for anyone to look into your issue more deeply, you'll probably need to create a repository that demonstrates the issue that people can poke at.

garyo commented 5 years ago

Sigh, you're right about needing to create a testcase repo. But it's now working for me as var deepmerge=require('deepmerge') so I'll have to find some time. Unit tests are run with vue-cli-service which runs jest. But indeed, that runs vue-jest and I don't know exactly what that does - possibly runs babel as well. (Would be handy to be able to run vue-jest on the cmd line and dump the result to a file.)

TehShrike commented 5 years ago

aah, yeah, I don't know anything about the semantics of Babel's import handling, that could be it.

isaachinman commented 5 years ago

@TehShrike I was very surprised to find that this package does not expose a proper ES module. I would expect any well-used and well-maintained modern JS package to transpile to multiple targets. Do you need help and/or a PR?

TehShrike commented 5 years ago

@isaachinman this package used to expose an ES entry point, but it was removed in #124 because of a bug in Webpack that caused issues for lots of people.

isaachinman commented 5 years ago

It's not a bug, you need a main and module in your package.json, each pointing to different transpilation targets.

Instead, it returns an object with default

But, that's exactly right. You cannot require an ESM module in the same way you require CJS. Perhaps this is a misunderstanding of how the two module systems work? It's easy enough to confuse - the landscape is not at all clear, especially when you are a package maintainer.

Moreover, you might be confused by Babel 5's compatibility hack and conflating that with Webpack. The maintainer of Webpack, @sokra, basically gave the same response a year ago.

I am happy to help if you're interested. Just know that maintainers of all JS packages have the same problem, and there are definitely solutions available.

Ultimately this package is 64 LOC and should be very simple to export for both ES and CJS.

TehShrike commented 5 years ago

The ESM entry point was rolled back because of this specific bug in Webpack, not because of any Babel behavior.

Webpack incorrectly uses the module entry point when it encounters require('any-library').

Lots of Webpack users hit the issue when one of their dependencies (that they didn't control) would be a CommonJS library containing a require('deepmerge') line that would bundle correctly for everyone else, but would bundle incorrectly for Webpack users.

isaachinman commented 5 years ago

Webpack incorrectly uses the module entry point when it encounters require('any-library')

That very well may be, but it's absolutely no reason to exclude an ES export from any library.

You don't seem willing to take that into account, and that's fine! It's a decision for a maintainer to make. I have already found another solution for my own use case.

For those dead set on using this package with Webpack and ES modules, you can do:

import { default as deepmerge } from 'deepmerge' 
TehShrike commented 5 years ago

That very well may be, but it's absolutely no reason to exclude an ES export from any library.

Did you read #87 or #97? If deepmerge publishes a module entry point with a default export, hundreds or thousands of consumers run into build errors. That is absolutely a reason for me to not bother with an ES export.

Trying to configure the exports of this library so that the CommonJS and ESM exports would be interpreted consistently by different bundlers would, at the very least, require a breaking change to the CommonJS exports, and I don't feel any motivation to do that.

You're in danger of coming off as a bit patronizing here. I'm pretty familiar with the module ecosystem, and you don't seem to have grasped the different types of bundler behaviors that resulted in those long issue threads in this project that I linked to above.

Other than it feeling correct and nice to expose an ESM entry point that doesn't have those few lines of UMD cruft (my motivation for exposing the ESM entry point in the first place), there's no huge benefit to implementing ESM in this library, since there's only small benefit to be gained from tree-shaking out the all function.

isaachinman commented 5 years ago

there's no huge benefit to implementing ESM in this library

Enough said!

verheyenkoen commented 5 years ago

Same problem. Used import * as deepmerge from 'deepmerge' as pointed out in the README and it fails. Just using import deepmerge from 'deepmerge' does the trick. So I guess the README should be updated.

TehShrike commented 5 years ago

@verheyenkoen what bundler? what version of deepmerge?

verheyenkoen commented 5 years ago

@TehShrike v3.2.0. The current version on npmjs.org (loaded with yarn but should be unrelated)

TehShrike commented 5 years ago

What bundler?

verheyenkoen commented 5 years ago

No bundler. Node.js

TehShrike commented 5 years ago

uh... node.js doesn't support ESM natively yet, does it?

That comes down to what magic are you using to get ESM to work (my first guesses are @std/esm or reify?)

If you're on node.js, just use const deepmerge = require('deepmerge')

verheyenkoen commented 5 years ago

I believe it's an experimental feature still but I use an esm library.

macdja38 commented 5 years ago

Since I think about 8.5 node has supported modules experimentally behind the flag --experimental-modules. Potentially a note about how to use it with the experimental system would be helpful so long as we update it if/when the experimental system changes.

TehShrike commented 5 years ago

node's built-in esm support is only relevant to packages that export an esm module, and even then I think only ones that use the mjs extension, so that's not relevant to deepmerge at the moment.

I suppose it makes sense to update the readme saying that you should only use import if you're using a bundler and you specifically don't like the CommonJS syntax.

macdja38 commented 5 years ago

@TehShrike no that doesn't really make sense. If you're using the experimental module system in a file you don't have a choice to fall back to require. It's one or the other. We should probably at least document how to use it from experimental-modules seeing as it already works perfectly. Or at least document that our strange import syntax doesn't apply to node experimental modules.

TehShrike commented 5 years ago

That sounds like a node.js problem, not a deepmerge problem – how are you supposed to use CJS modules from inside a node ESM module? I don't know the answer.

Bear in mind that @verheyenkoen is talking about something completely different, since he's using the esm library to achieve his importing goals, and can absolutely use require in his case.

macdja38 commented 5 years ago

@TehShrike No no, you can import CJS modules, just not using require, using import export syntax... and it's not a node.js problem, node.js actually has a completely reasonable api for importing our module.

Literally import deepmerge from 'deepmerge' it's just that our documentation recommends doing something else if you're using import/export syntax as a blanket statement instead of saying only in web-pack / bundles.

bfintal commented 5 years ago

Ran into this same issue while running Jest on my modules. import deepmerge from 'deepmerge' fixed the issue as suggested above by @verheyenkoen 👍