Fix broken default export when required from CJS #2148
Conversation
When this package was converted to TS, `module.exports = ` was converted to `export default`, without taking into account that this would TS emit this as a named export of "default" (`exports.default = sideWatch`), so CJS consumers would have to call it as `require('@embroider/broccoli-side-watch').default()` (aka "double default" problem). As we are only targeting CJS consumers, this changes makes the compiled js have a single main export as `module.exports = sideWatch`.
| @@ -27,7 +27,7 @@ interface SideWatchOptions { | |||
| dependencies that you're actively developing. For example, right now | |||
| @embroider/webpack doesn't rebuild itself when non-ember libraries change. | |||
| */ | |||
| export default function sideWatch(actualTree: InputNode, opts: SideWatchOptions = {}) { | |||
| function sideWatch(actualTree: InputNode, opts: SideWatchOptions = {}) { | |||
There was a problem hiding this comment.
I believe it's better to author as proper modules and expose a compiled dist instead of source than to ship cjs compatible code by default
There was a problem hiding this comment.
Not sure what exactly you mean here? How could I write code or set up TS so that export default compiles to something that would work in CJS as const sideWatch = require('@embroider/broccoli-side-watch')?
FWIW, we have a similar case here.
The alternative would be to not using a default export, but require users to const { sideWatch } = require('@embroider/broccoli-side-watch'), which wouldn't suffer from that interoperability problem.
There was a problem hiding this comment.
personally I don't see why import { sideWatch } ... is frowned upon
I would prefer that to writting the similar case you mentioned before
while this will not really be needed in vite land and so on
it sets precedence which could be followed when adding other packages that may be used from module only places
and I'm not sure if we're not going to have issues with macros in the future because of the choice you mentioned there in systems that use modules exclusively
are we in a new enough node environment where we could say use top level import? would that work?
as for how to make the current syntax work, I'm talking ab out shipping two separate bundles one for cjs and one for modules with entry points specific to each declared in package json
all of that is a much more convoluted setup and would require more of a pre build step than we're doing now
There was a problem hiding this comment.
This package is going to be used in ember-cli-build.js, which is supposed to be CJS, so I think it does need to work nicely in a CJS env.
I'm open to moving to const { sideWatch } = require('@embroider/broccoli-side-watch'), but since this package existed before (as unstable) and was actively used, I preferred to not cause a breaking change.
For the dual build setup, I'm all for that, but indeed this is well out of scope for this quick fix.
When this package was converted to TS,
module.exports =was converted toexport default, without taking into account that this would make TS emit this as a named export of "default" (exports.default = sideWatch), so CJS consumers would have to call it asrequire('@embroider/broccoli-side-watch').default()(aka "double default" problem). As we are only targeting CJS consumers, this changes makes the compiled js have a single main export asmodule.exports = sideWatch.Tests weren't able to catch this, as TS was providing the interoperability, while node.js does not.