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

Exported mutable variable in namespace with setter function does not mutate value #5259

Closed
abrenneke opened this issue Jul 20, 2022 · 4 comments · Fixed by #7202
Closed

Exported mutable variable in namespace with setter function does not mutate value #5259

abrenneke opened this issue Jul 20, 2022 · 4 comments · Fixed by #7202
Labels
Milestone

Comments

@abrenneke
Copy link

abrenneke commented Jul 20, 2022

Describe the bug

If an export from a namespace is mutable, and a function in the namespace mutates the value, the exported value is not mutated.

Input code

namespace A {
    export let a = 5;

    export function setA(value: number) {
        a = value;
    }
}

Config

{
  "jsc": {
    "parser": {
      "syntax": "typescript",
      "tsx": false
    },
    "target": "es5",
    "loose": false,
    "minify": {
      "compress": false,
      "mangle": false
    }
  },
  "module": {
    "type": "commonjs"
  },
  "minify": false,
  "isModule": true
}

Playground link

https://play.swc.rs/?version=1.2.218&code=H4sIAAAAAAAAA8tLzE0tLkhMTlVwVKjmUgCC1IqC%2FKIShZzUEoVEBVsFU2suZOG00rzkksz8PIXi1BJHjbLEnNJUK4W80tyk1CJNqAEgANIJlrQGC9Vy1QIA8jab8GkAAAA%3D&config=H4sIAAAAAAAAA0WNSwrDMAxE76J1tt3kDj2ESZXg4B8aBWqM7165OGQnzedNoxMbrY2KE7CMCzWp%2B9JKWgtjE1%2BUFlKYtLsA7vY4OVgtwniZF3IGT3eh6JPf6yBtORZh4LFcOsKd7AaK%2BXMNof3HDGiVmNMJ6g9olj3eM61ycf8BnwbyVboAAAA%3D

Expected behavior

tsc output correctly mutates the value:

"use strict";
var A;
(function (A) {
    A.a = 5;
    function setA(value) {
        A.a = value;
    }
    A.setA = setA;
})(A || (A = {}));

Actual behavior

Mutates the variable internal to the namespace but not the exported value from the namespace.

"use strict";
var A;
(function(A) {
    var setA = function setA(value) {
        a = value;
    };
    var a = A.a = 5;
    A.setA = setA;
})(A || (A = {}));

Version

1.2.218

Additional context

No response

@kwonoj
Copy link
Member

kwonoj commented Jul 20, 2022

Afaik this is expected by we try to model similar approach to babel where we compile each module as isolated.

https://babeljs.io/docs/en/babel-plugin-transform-typescript

Q: Why doesn't Babel allow export of a var or let?

A: The TypeScript compiler dynamically changes how these variables are used depending on whether or not the value is mutated. Ultimately, this depends on a type-model and is outside the scope of Babel. A best-effort implementation would transform context-dependent usages of the variable to always use the Namespace.Value version instead of Value, in case it was mutated outside of the current file. Allowing var or let from Babel (as the transform is not-yet-written) is therefore is more likely than not to present itself as a bug when used as-if it was not const

Babel raises compile time exception though, while we do not fail but does not allow those behavior.

I may wrong, so will leave issue opened until getting second eye on this.

babel repl for ref: https://babeljs.io/repl/#?browsers=defaults%2C%20not%20ie%2011%2C%20not%20ie_mob%2011&build=&builtIns=false&corejs=3.21&spec=false&loose=false&code_lz=HYQwtgpgzgDiDGEAEBBJBvAUEnSIA8YB7AJwBckAbCCkJAXiQFYBuTbXA48pAMwFdg8MgEsiwJFBooAFADcQlfhABcSYPzAAjCCQCUGDrlx1GCpRDbGAvpmtA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env%2Creact%2Cstage-2%2Ctypescript&prettier=false&targets=&version=7.18.9&externalPlugins=&assumptions=%7B%7D

@abrenneke
Copy link
Author

abrenneke commented Jul 20, 2022

Yeah that makes sense, babel doesn't even try to compile it so 🤷 - swc does compile it so I was surprised when a call simply didn't work at all, would rather it fail instead of code just running differently though.

@kdy1 kdy1 added this to the Planned milestone Jul 21, 2022
@kdy1
Copy link
Member

kdy1 commented Dec 22, 2022

It's consistent with behavior of tsc with isolatedModules: true

@kdy1 kdy1 removed this from the Planned milestone Dec 22, 2022
@swc-bot
Copy link
Collaborator

swc-bot commented Apr 30, 2023

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.

@swc-project swc-project locked as resolved and limited conversation to collaborators Apr 30, 2023
magic-akari added a commit to magic-akari/swc that referenced this issue Sep 19, 2023
@kdy1 kdy1 added this to the Planned milestone Sep 25, 2023
kdy1 pushed a commit that referenced this issue Sep 25, 2023
**Description:**

## Bugfixes
- Exported `let`/`var` declarations in TypeScript namespaces should be mutable.
- Fix missing declaration of complex exported patterns in TypeScript namespaces.
- Preserve concrete TS namespaces.

## New Features
- Introducing [Verbatim Module Syntax](https://www.typescriptlang.org/tsconfig#verbatimModuleSyntax).
- Enum value will now be inlined whenever possible within a single module, optimizing runtime performance.
- Constant enums will be automatically eliminated when feasible, This can reduce bundle size.
- Added support for cross-referencing enum values.

## Deprecated
`TsEnumConfig` is deprecated
  - The `treat_const_enum_as_enum` transform option is deprecated.
  - The `ts_enum_is_readonly` assumption option is deprecated.

**BREAKING CHANGE:**

TypeScript Config is changed.


**Related issue:**

 - Closes #5197
 - Closes #5259 
 - Closes #7177
 - Closes #7453
 - Closes #7676 
 - Closes #7681
 - Closes #7791 
 - Closes #7961
@kdy1 kdy1 modified the milestones: Planned, v1.3.89 Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

4 participants