swc-project / swc

Rust-based platform for the Web
https://swc.rs
Apache License 2.0
31.23k stars 1.23k forks source link

Function name unnecessarily (and possibly wrongly) renamed #1600

Closed pbadenski closed 3 years ago

pbadenski commented 3 years ago

Describe the bug

const x = function a() {};

const y = function a() {};

After running swc test.ts:

"use strict";
const x = function a() {
};
const y = function a1() {
};

Wondering whether SWC should preserve second function name as a - as intended in the original code? (Babel does) In fact I'm not quite sure why SWC is renaming second function name at all.

Config

{
  "env": {
    "targets": {
      "node": "15.10"
    }
  },
  "jsc": {
    "target": "es2019",
    "parser": {
      "syntax": "typescript",
      "tsx": true
    }
  },
  "module": {
    "type": "commonjs"
  }
}

Version

@swc/cli: 0.1.39
@swc/core: 1.2.54
devongovett commented 3 years ago

You probably shouldn't rely on the function name for your code to work correctly. Most minifiers rename them anyway.

pbadenski commented 3 years ago

I get that, this popped in a bit artificial scenario in the test code. However - it's surprising enough for me to think this might be uncovering some lurking oddity.

devongovett commented 3 years ago

The references to the function should also have been renamed so it should be ok. The reason is to avoid duplicate bindings of the same name. It could perhaps be optimized to avoid renaming in more cases, but the code should at least work.

jdb8 commented 3 years ago

Ran into this myself when wondering why Enzyme tests started failing after transpilation: turns out that Babel (seemingly as part of preset-env - repl link) will take code like:

var Foo = (props) => {};

and output

var Foo = function Foo(props) {};

with a named function. If you then pass this through swc, it will rename the function to Foo1:

var Foo = function Foo1(props) {};

This causes Enzyme tests to fail because React displayNames, when not set, are based on the function name here: so a function that Babel was reliably producing as Foo would now be seen in the React dom as Foo1. Arguably code shouldn't be relying on this behaviour, but it was surprising and is making it tricky to migrate from Babel to swc seamlessly.

Updating code to instead just declare function Foo instead of assigning to an anonymous arrow function works to "fix" it, but it means updating lots of packages which may be implicitly relying on the way Babel transpiles things and so are not setting displayName explicitly.

FWIW this happens without any config passed to swc. Weirdly, this behaviour doesn't seem to repro in the online swc repl (https://swc.rs/repl) - not sure if there's an older or newer version running there which differs from my local version (1.2.74).

RyannGalea commented 3 years ago

Was this added to v1.2.79 ?

kdy1 commented 3 years ago

Yes, because this requires some work. I decided to postpone this when I'm releasing v1.2.78

RyannGalea commented 3 years ago

@kdy1

After updating and trying again, it does what I was originally asking but not adding the '1' to all my classes but it seems like something else is happening with decorators. as I get this error when I call the server. I'm not sure if I'm missing any other config for Typescript & as I mentioned using babel I used to have to compile to esnext, then back to es5 to make it work.

[GraphQLError [Object]: Service with "<UNKNOWN_IDENTIFIER>" identifier was not found in the container. Register it before usage via explicitly calling the "Container.set" function or using the "@Service()" decorator.] {
@Service()
@Resolver()
export class LoggedUserQuery {

  constructor(private readonly us: UserService){ }

  @Query(returns => User)
  @CatchAll('Logged User Error')
  async loggedUser(
    @Ctx() { req },
    @CtxUser() { _id }
  ): Promise<User | null> {
    if (!_id) return null
    const user = await this.us.findOne({_id}, ['business'])
    req.session.user = user
    return user
  }

} 
{
  "jsc": {
    "parser": {
      "syntax": "typescript",
      "tsx": false,
      "decorators": true,
      "dynamicImport": true
    },
    "target": "es5",
    "loose": true,
    "transform": {
      "legacyDecorator": true,
      "decoratorMetadata": true
    },
    "experimental": {
      "optimizeHygiene": true
    }
  },
  "module": {
    "type": "commonjs"
  },
  "sourceMaps": true
} 

I did have this in the config earlier but I don't know if this was actually doing anything as I never got it working because of the '1' issue. If I add this back in, the '1' issue comes back anyway currently.

"env": {
  "targets": {
    "node": "14"
  }
}
jdb8 commented 3 years ago

@kdy1 thanks, this option resolved a lot of the cases I was running into.

However, I noticed that this code somehow gets renamed, seemingly because of the variable name Option:

> require('@swc/core').transformSync('var Option = "hi";', {"module": {"type": "commonjs"}, "jsc": {"experimental": {"optimizeHygiene": true}}})
{ code: '"use strict";\nvar Option1 = "hi";\n' }

I initially thought this was the new optimizeHygiene flag, but it seems to repro even without that:

> require('@swc/core').transformSync('var Option;')
{ code: 'var Option1;\n' }
> require('@swc/core').transformSync('var option;') // this one's fine
{ code: 'var option;\n' }

So I suppose this should be a separate issue, if it isn't already. I can't repro this on https://swc.rs/repl (but I also couldn't repro the original issue there either to be fair). Wondering if this could have been introduced with the latest variable mangling changes?

EDIT: ah, is it related to js globals? I can repro this for Math too:

> require('@swc/core').transformSync('var Math;')
{ code: 'var Math1;\n' }
swc-bot commented 2 years ago

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.