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

ES2015 and up don't handle ESM default exports correctly #5768

Closed
lgibso34 opened this issue Sep 6, 2022 · 9 comments
Closed

ES2015 and up don't handle ESM default exports correctly #5768

lgibso34 opened this issue Sep 6, 2022 · 9 comments
Labels

Comments

@lgibso34
Copy link

lgibso34 commented Sep 6, 2022

Describe the bug

I ran into this when using @swc/jest. The error is ReferenceError: Cannot '_default' before intialization

const seems to have start being used in 1.2.212 causing this issue. Previous versions were using var which gets hoisted.

image

Input code

No response

Config

No response

Playground link

https://play.swc.rs/?version=1.2.212&code=H4sIAAAAAAAAA0utKMgvKlFISU1LLM0pUdDQVLC1U6jm5VIAgqLUktKiPAWljNScnHwlXq5aAErKsCUtAAAA&config=H4sIAAAAAAAAA0WOSwrDMAxE76J1Fm2hG9%2BhhzCuEhz8Q%2BNATfDdK4eE7Ib5PGmnFY7MTsUKWIZCS9X%2ByFBtheHEl0oTVahVZeOu2srCVRuM1%2BP51jTkDCYz2wCeKPrk5zZYLsciDNyRTUu4ml1ZMX%2B3YezHOWXqJOa0gvoNOscen7N9PPIHEdWl27wAAAA%3D

Expected behavior

The code should compile correctly in a usable manner and not error.

Actual behavior

No response

Version

1.2.212

Additional context

No response

@lgibso34 lgibso34 added the C-bug label Sep 6, 2022
@kdy1 kdy1 added this to the Planned milestone Sep 6, 2022
@magic-akari
Copy link
Member

The error occurred because of TDZ which is expected.
We try to keep this feature when transformed from ESM to CJS.

See this example, run it in node native ESM environments, you will get the error.

import foo from "./index.js"; // self import

console.log(foo); // ReferenceError: Cannot access 'foo' before initialization

export default () => {
    return "hello";
};

move the console.log as following.

import foo from "./index.js"; // self import

export default () => {
    return "hello";
};

console.log(foo); // [Function: default]

@kdy1
Copy link
Member

kdy1 commented Sep 6, 2022

I tested using chrome but

Object.defineProperty(window, "__esModule", {
    value: true
});
Object.defineProperty(window, "default", {
    enumerable: true,
    get: ()=>_default
});
const _default = ()=>{
    return "hello";
};

worked.
So this issue is invalid

@kdy1 kdy1 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 6, 2022
@kdy1 kdy1 removed this from the Planned milestone Sep 6, 2022
@kdy1 kdy1 added invalid and removed C-bug labels Sep 6, 2022
@lgibso34
Copy link
Author

lgibso34 commented Sep 6, 2022

I'll try to create a repo where I am experiencing this.

I ran into this at work so I'll have to try and recreate this on my home machine.

My issue was actually the export default MyComponent part. If I removed that it worked.

@lgibso34
Copy link
Author

lgibso34 commented Sep 7, 2022

Okay here is a repo to reproduce the issue.
I do wonder if this is actually an issue or just a limitation with CommonJS now that I understand what is happening. I think @magic-akari is right about the TDZ issue. I look forward to understanding this from you all's perspective.

Files of interest:

  1. So temp.tsx default exports a React component.
  2. That React component is imported in constants.tsx and placed in an object.
  3. temp.tsx imports somethingElse from constants.tsx but in CommonJS (what Jest uses) technically the JS object that <Temp> is stored in is also "pulled in" or whatever the term is. (no tree shaking like ESM)
  4. This causes the ReferenceError: Cannot '_default' before intialization

Is this just expected with CommonJS?

IMPORTANT:
This code WORKS if you change the jsc target to es5 (not what I want to do). And of course it works with babel before trying out swc. I would expect swc to just work as a drop in replacement for babel and I would expect it to work for ES2015 and up.

Thanks for looking!

EDIT: Well heck, is there any benefit to choosing the target when compiling for Jest? Like what benefit do you get from using es2022 over es5?
Also, is it normal to get different code coverage when switching to swc? My numbers are way different.

@magic-akari
Copy link
Member

@lgibso34 try this https://gist.github.com/magic-akari/5a8a1bee1c78aa94b383572bed2ce44e
clone and run npm test, you will get the same error in native ESM environment.

This line is where the error is located.
https://github.com/lgibso34/swc-issue/blob/361404abc0f8aba226ecfa6d9f05af5f8e7d0202/src/constants.tsx#L4

You can get rid of the error by using the lazy trick.

const someObj = {
  Temp: ()=> Temp,
};

@lgibso34
Copy link
Author

lgibso34 commented Sep 7, 2022

Thank you for spending the time on this. I appreciate your feedback. The lazy trick is a good solution.

Could you explain why this is happening? Like it works when you set the target to ES5 and it worked with babel previously.
Could you answer some of my questions above just so I have a better understanding?

I'd like to leave here with complete understanding since to me it "seems" like an issue that it works with ES5 but not with ES2015 and up. I know now that it may not be an issue with swc but I'd like fully be aware of why.

Thanks again.

@magic-akari
Copy link
Member

The conversion from ESM to CommonJS done by esbuild attempts to preserve the behavior of the original code as much as possible. What you are observing happens because in ESM, all imports are essentially "hoisted" to the top of the file since all dependencies of a given file must be evaluated before any code in that file is evaluated. This is just how the JavaScript language works. More information: evanw/esbuild#1395 (comment).

The code you wrote happens to work due to a defect in the TypeScript compiler: microsoft/TypeScript#16166 (comment). Writing such code is risky because your code will break as soon as it's run in a real ESM environment (such as esbuild, or node's ESM mode, or in a browser).

Originally posted by @evanw in evanw/esbuild#1444 (comment)

This is just how import and export in JavaScript works. An import or export statement is only a declaration that the file either has dependencies or exposes live bindings to dependents. They just declare properties about the file and are not evaluated inline. At a high level, it works something like this (ignoring complexities such as top-level await):

  1. Order all module files to be evaluated such that dependencies come before dependents
  2. Use import and export to replace references to imports with references to the underlying exports
  3. Ignore all import and export statements with paths since they have now served their purpose
  4. Evaluate all code in the order in step 1

Originally posted by @evanw in evanw/esbuild#1395 (comment)

We can follow these steps to understand how ESM works.

For the following input. ( Removed the React content because they are not relevant. )

// const.js
import Temp from "./temp";

const someObj = {
  Temp, // because this is importing Temp and Temp is importing somethingElse it hits this error
};

export const somethingElse = "not using Temp but commonJS does not tree shake";
// temp.js
import { somethingElse } from "./const.js";

const Temp = () => {
  return somethingElse;
};

export default Temp;
export { Temp };
// index.js
import temp from "./temp.js"

And put them togeter by those steps. we get the following codes.

const someObj = {
  Temp, // because this is importing Temp and Temp is importing somethingElse it hits this error
};

const somethingElse = "not using Temp but commonJS does not tree shake";

const Temp = () => {
  return somethingElse;
};

It is easy to see that this is not valid.

Note: Changing const Temp to var Temp does not work in ESM, since there is no way to change import binding behaviour.


How to get rid of this error?

  1. lazy trick
  2. function hoist

The following code works as expected because of the function hoist.

const someObj = {
  Temp, // because this is importing Temp and Temp is importing somethingElse it hits this error
};

const somethingElse = "not using Temp but commonJS does not tree shake";

function Temp() {
  return somethingElse;
};

Function hoist works in ESM as well.

export default function Temp() {
  return somethingElse;
};

export { Temp };

Note: export default expr and export default function fn_decl are different.

@magic-akari
Copy link
Member

When you write import/export statements, because they are ESM syntax, swc attempts to preserve the behavior of the ESM feature.

If you do not need / do not care the ESM behaviour, try the following.

import foo = require("./foo");  // CTS syntax, cjs require in TypeScript

function x(){
}

// `module.exports = ` in CTS
export = {
    bar: 42,
    x,
}

@github-actions
Copy link

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

3 participants