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

esModule: false fix read only TypeError in exports #6758

Merged
merged 1 commit into from
May 29, 2019
Merged

esModule: false fix read only TypeError in exports #6758

merged 1 commit into from
May 29, 2019

Conversation

cif
Copy link
Contributor

@cif cif commented May 28, 2019

As suggested from previous PR, any client exclusively using react-router-dom package via CommonJS modules will run into this problem:

    TypeError: Cannot assign to read only property '__esModule' of object '[object Object]'

      at node_modules/react-router-dom/cjs/react-router-dom.js:288:64
          at Array.forEach (<anonymous>)
      at Object.<anonymous> (node_modules/react-router-dom/cjs/react-router-dom.js:288:26)
      at Object.<anonymous> (node_modules/react-router-dom/index.js:6:20)

Changing rollup config to omit Object.defineProperty(exports, '__esModule', { value: true }); will fix this, however my suggestion in #6757 seems less heavy handed in that it doesn't break interop https://stackoverflow.com/a/55687758

@cif cif changed the title esModule: false fix read only TypeError in expors esModule: false fix read only TypeError in exports May 28, 2019
@cif
Copy link
Contributor Author

cif commented May 29, 2019

Maybe interop wasn't the best idea anyway¯_(ツ)_/¯ https://medium.com/@dlmanning/interoperability-between-es-modules-and-common-js-is-a-mistake-fb9ac71d96fd

@timdorr
Copy link
Member

timdorr commented May 29, 2019

OK, I'm down with this.

One thing: Can you actually run the build and update the bundle size snapshots? This will change them.

@cif
Copy link
Contributor Author

cif commented May 29, 2019

When I ran npm build to test locally that didn't seem to produce any other artifacts. I just went through and deleted the .size-snapshot.json files, re-ran npm build and they appeared to produce unchanged versions of master, so there's nothing to commit. Without diving in more, seems the way the reports are configured the single line difference in CJS export isn't turned up?

@timdorr
Copy link
Member

timdorr commented May 29, 2019

Oh duh, this is only for the CJS build, which isn't tracked by that file. We're all good then.

@timdorr timdorr merged commit caa9950 into remix-run:master May 29, 2019
@cif
Copy link
Contributor Author

cif commented May 29, 2019

Awesome thank you! This was blocking us from updating to v5 on a new project using Jest. I'll hang back on 4 until it's released but hopefully that does the same for others.

Really appreciate the hard work and dedication to OSS projects like this one @timdorr !

@lock lock bot locked as resolved and limited conversation to collaborators Jul 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants