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

Help using deepmerge as module #137

Closed
garyo opened this issue Feb 9, 2019 · 25 comments
Closed

Help using deepmerge as module #137

garyo opened this issue Feb 9, 2019 · 25 comments

Comments

@garyo
Copy link

garyo commented Feb 9, 2019

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
Copy link
Owner

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
Copy link
Author

garyo commented Feb 9, 2019

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
Copy link
Owner

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
Copy link
Author

garyo commented Feb 9, 2019

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
Copy link
Owner

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

@isaachinman
Copy link

@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
Copy link
Owner

@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
Copy link

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
Copy link
Owner

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
Copy link

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
Copy link
Owner

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
Copy link

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

Enough said!

@verheyenkoen
Copy link

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
Copy link
Owner

@verheyenkoen what bundler? what version of deepmerge?

@verheyenkoen
Copy link

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

@TehShrike
Copy link
Owner

What bundler?

@verheyenkoen
Copy link

No bundler. Node.js

@TehShrike
Copy link
Owner

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
Copy link

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

@macdja38
Copy link
Collaborator

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
Copy link
Owner

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.

TehShrike added a commit that referenced this issue Mar 18, 2019
Inspired by the recent discussion in #137
@macdja38
Copy link
Collaborator

@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
Copy link
Owner

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
Copy link
Collaborator

@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
Copy link

bfintal commented Jun 5, 2019

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

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

No branches or pull requests

6 participants