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

[REGRESSION] Cannot assign to read only property '__esModule' of object #14890

Closed
appsforartists opened this issue Mar 28, 2017 · 14 comments
Closed
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@appsforartists
Copy link

TypeScript Version: 2.2.1

Code

// presuming [email protected] is installed
import Link from 'react-router/Link';

Note: Using [email protected] to do bundling

tsconfig.json

{
  "compilerOptions": {
    "declaration": true,
    "lib": [
      "es2015",
      "es2016",
      "es2017",
      "dom"
    ],
    "importHelpers": true,
    "module": "commonjs",
    "moduleResolution": "node",
    "noImplicitAny": true,
    "noImplicitThis": true,
    "sourceMap": true,
    "strictNullChecks": true,
    "target": "es6"
  }
}

Expected behavior:

Link is imported to the local namespace.

Actual behavior:

Uncaught TypeError: Cannot assign to read only property '__esModule' of object '#<Object>'
    at Object.<anonymous> (Link.js:3)

in Chrome 56.0.2924.87. Reverting to [email protected] solves the issue.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 28, 2017

__esModule property is emitted by the compiler in any ES6 module. This tells other transpiles that this module follows the ES6 module semantics and not a CommonJS/AMD module.

Why are you assigning to __esModule property?

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Mar 28, 2017
@appsforartists
Copy link
Author

I'm not - tsc is.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 28, 2017

Can you share a self contained sample showing the issue?

@appsforartists
Copy link
Author

Looks like this might be an incompatibility between [email protected] and Babel. As you can see, Babel is defining __esModule:true in its exports:

I had surmised that tsc had changed its behavior around __esModule since this is a regression in 2.2, but upon investigating further, I see that __esModule is present in packages that have been built with babel and published to npm.

@appsforartists
Copy link
Author

appsforartists commented Mar 28, 2017

Let me know if this is helpful; otherwise, I can delete it:

https://github.com/material-motion/tsc-reduction

@mhegazy
Copy link
Contributor

mhegazy commented Mar 28, 2017

What do i need to run to trigger the error?

@appsforartists
Copy link
Author

I just pushed a sample page to that repo. Pull the latest and run yarn run start, then open it in Chrome.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 28, 2017

Looks like your loader is calling the ts compiler on a .js file ("$app/node_modules/react-router/Link.js) that has already been transformed. not sure why though.

@appsforartists
Copy link
Author

Sounds like it's mostly a configuration issue, though it's bizarre that it worked and now it doesn't. Did TS2.2 make meaningful changes to __esModule? That's not listed in https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript

@mhegazy
Copy link
Contributor

mhegazy commented Mar 29, 2017

yes. In TS 2.1 __esModule was only emitted in modules that had a default export, in TS 2.2 __esModule for all modules.

appsforartists added a commit to material-motion/material-motion-js that referenced this issue Mar 29, 2017
Summary: to workaround microsoft/TypeScript#14890

Reviewers: O2 Material Motion, #material_motion, featherless

Reviewed By: O2 Material Motion, #material_motion, featherless

Tags: #material_motion

Differential Revision: http://codereview.cc/D2970
appsforartists added a commit to material-motion/material-motion-js that referenced this issue Apr 10, 2017
Summary: Working around microsoft/TypeScript#14890 by only running TS files through TypeScript

Reviewers: O2 Material Motion, O3 Material JavaScript platform reviewers, #material_motion, featherless

Reviewed By: O2 Material Motion, #material_motion, featherless

Tags: #material_motion

Differential Revision: http://codereview.cc/D3031
@mhegazy mhegazy closed this as completed Apr 21, 2017
@appsforartists
Copy link
Author

@DanielRosenwasser - you were interested in this.

@appsforartists
Copy link
Author

@mhegazy @DanielRosenwasser Is this something we can revert?

It used to be safe to run your entire node_modules folder through TypeScript, which meant you could use the same tool (tsc) to translate your whole app to any ES version. This change broke that ability.

ES6 is supported in all major browsers, but developers often still publish in ES5, both out of habit and out of concern for legacy users. There are many negative ramifications:

  • simple expressions like generators explode into complex state machines;
  • each library might include a different polyfill, which means more code shipped to users to support the same feature;
  • translated code may run slower, and developers have no way to opt back-in to the modern version; and most importantly
  • some features like Custom Elements don't work correctly when translated.

The web development community would be in a better spot if we had a culture that allowed module authors to publish code in its original format and let application authors be concerned with translating/polyfilling for old browsers. In order to make that shift, it needs to be safe for an application author to run a transpiler on any arbitrary node_modules, which means this change (always injecting __esModule without checking if one has already been set) needs to be reverted.

@mhegazy
Copy link
Contributor

mhegazy commented May 1, 2017

@mhegazy @DanielRosenwasser Is this something we can revert?

No. These changes are done in preparation for node commonjs interop.

which means this change (always injecting __esModule without checking if one has already been set)

do you have an sample package we can look at?

@appsforartists
Copy link
Author

@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

2 participants