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

fix: use named "default" export option for CJS build (BREAKING CHANGE) #40

Closed

Conversation

billiegoose
Copy link

This PR bring the CJS and ESM modules into alignment, so that import memoize from 'memoize' works correctly whether in TypeScript, Jest, or Webpack. It does this by modifying the CJS module to have an export named "default", which is literally how ESM default exports work.

Fixes #37

BREAKING CHANGE: Node users will have to use var memoize = require('memoize').default from now on.

@alexreardon
Copy link
Owner

Thoughts @TrySound?

@TrySound
Copy link
Contributor

TrySound commented Nov 18, 2018

@wmhilton @alexreardon I would move typescript state to less noisy for non-ts projects and just use esModuleInterop flag which fixes the problem perfectly and do not create issues like this one.
We moved with this way in material-ui-pickers because of date-fns which has exactly same issue.

Sticking on TS legacy makes community very isolated. They had a chance to make esModuleInterop as default in v3.

@billiegoose
Copy link
Author

@TrySound This isn't a TypeScript problem. This is a "your CJS module implements a totally different API than your ES module".

@billiegoose
Copy link
Author

It makes it very difficult to write isomorphic code that works seamlessly in both Node and Webpack and Jest.

@TrySound
Copy link
Contributor

TrySound commented Nov 19, 2018

esModuleInterop simplifies everything.
Well, node has a million modules with normal modules.exports = () => {}.
TypeScript forces to import them as import * as fn from 'fn'; fn().
Rollup will fail because "why the hell are you gonna call module namespace?".
You can't fix all node modules. You may fix TypeScript.

@TrySound
Copy link
Contributor

Rollup is semantically right here. TypeScript forces doubtful decision by default.

@billiegoose
Copy link
Author

billiegoose commented Nov 19, 2018

Eh. Then let's close this PR and add update the README with TypeScript instructions so fools like me don't keep making the same mistakes that drive everyone towards #37 Because it does appear that this magic esModuleInterop config thing works. (I'd never heard of it before.)

@alexreardon
Copy link
Owner

I am happy to defer to @TrySound here. I think the best path forward for now is to add the TypeScript guide to the docs

@alexreardon
Copy link
Owner

@TrySound are we merging this one, or just updating the docs?

@TrySound
Copy link
Contributor

I'm all for docs. We should enforce community with safer usage. Not necessary config less.

@alexreardon
Copy link
Owner

Given your familiarity with the problem space, could you create a PR @TrySound with a short note about esModuleInterop?

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.

None yet

3 participants