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

Buffer got wrong value when "downlevelIteration":"true" in tsconfig.json #760

Closed
JaosnHsieh opened this issue Jul 16, 2020 · 2 comments
Closed
Labels
solution: duplicate This issue or pull request already exists solution: workaround available There is a workaround available for this issue topic: downlevelIteration Related to tsconfig downlevelIteration

Comments

@JaosnHsieh
Copy link

JaosnHsieh commented Jul 16, 2020

Current Behavior

$ tsdx create -> basic

Node.js Buffer object with spread operator has wrong value.

Screen Shot 2020-07-16 at 4 49 48 PM

tsconfig.json

{
  "include": ["src", "types"],
  "compilerOptions": {
    "module": "esnext",
    "lib": ["dom", "esnext"],
    "importHelpers": true,
    "declaration": true,
    "sourceMap": true,
    "rootDir": "./src",
    "strict": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "noImplicitReturns": true,
    "noFallthroughCasesInSwitch": true,
    "moduleResolution": "node",
    "baseUrl": "./",
    "paths": {
      "*": ["src/*", "node_modules/*"]
    },
    "jsx": "react",
    "esModuleInterop": true,
    "downlevelIteration": true
  }
}

Expected behavior

Got value Buffer <[1,2,3,4,5,6]>

Suggested solution(s)

might be rollup config. because below tsconfig.js without tsdx transpiled and got values correctly. so at least it should not be typescript bug.

I avoid using spread operator ... on Buffer. Switch to ...Buffer.from([1,2,3]).toJSON().data for now as workaround.

Screen Shot 2020-07-16 at 5 00 28 PM

below tsconfig.json with typescript Version 3.9.6 works

{
  "compilerOptions": {
    "target": "es2018",
    "module": "commonjs",
    "outDir": "dist",
    "lib": ["es2018", "esnext.asynciterable", "DOM"],
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "esModuleInterop": true,
    "downlevelIteration": true
  }
}


Additional context

Your environment

Software Version(s)
TSDX 0.13.2 with tsdx create -> basic
TypeScript 3.9.6
Browser
npm/Yarn 1.22.4
Node v13.7.0
Operating System MacOS Catalina 10.15.5 (19F101)
@agilgur5
Copy link
Collaborator

Looks like a duplicate of #659 and #376 . There's a few workarounds mentioned there as well.

As a permanent fix for this, I think we want to turn off loose mode in Babel, which I've been looking into. It is slightly breaking but should generally be safe to make things more strict to the ECMA standard.

@agilgur5 agilgur5 added solution: duplicate This issue or pull request already exists solution: workaround available There is a workaround available for this issue labels Jul 16, 2020
@agilgur5 agilgur5 added the topic: downlevelIteration Related to tsconfig downlevelIteration label Jul 18, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 15, 2020

So I looked into changing loose a good bit and got some confusing results. Here's a summary:

First use of loose internally

This was first introduced in #117. I'm not sure why loose was turned on there as it's not mentioned in PR or commit description and there are no comments.

I know the old Babel 6 stage-* presets used loose mode, and so the migration guide included turning those on (but not preset-env itself?) and I've got some boilerplate that did so too, so maybe that's why? Unsure.

It seems more likely that it was taken from microbundle, especially as all the surrounding code is quite similar.

The default in Babel 7 is false though, and this change is mentioned in the Babel 7 upgrade guide.

Controversial option

Here's a Twitter thread by microbundle's author recommending the usage (maybe that's where TSDX's usage came from?), but mostly for optimization. I personally think correctness is more important than optimization; doesn't matter how fast the code is if it doesn't compile correctly or do what it's supposed to.

Also one of Babel's original authors said "Loose mode was a mistake" in that thread.

downlevelIteration is also false by default in tsconfig.json. This may be due to backwards compat however, and not for any particular bug or optimization issue, but it has no recommendation to be true at the same time (although these lag behind, I've contributed a good bit to the TS docs so know that first-hand).

From this, seemed ok to switch, and any perf degradation would be reduced by dedupe, gzip, etc. It also may not transpile the same way anymore given the more modern browser outlook. But then found some potential issues with turning loose to false...

Potential for bugs

So while the suggestion is that it shouldn't break anything by being closer to the ECMA standard, I saw some discussions that challenged that theory.

In particular, I looked at CRA's Babel config, which sets class-properties's loose option to true. It references in a comment facebook/create-react-app#4263 which suggests poor perf impact. PR facebook/create-react-app#4248 points to a bug/issue with MobX's use of decorators mobxjs/mobx#1471. Decorator proposals have been stalled for a while so seems like this workaround may still be necessary in the meantime then.

It does not set preset-env itself's loose mode though, so this suggests class-properties may be the only issue. The stage-* migration not including it adds credence to that as well.

Next Steps

  1. Should log warnings for all tsconfig.json options that TSDX does not support or must be configured via @babel/preset-env instead. This includes downlevelIteration, target, outDir, etc. I've been considering adding comments to the default tsconfig.jsons generated by tsdx create for a while now, but logging on real usage for all users is actually a better option (though perhaps both should be done).

    • We may want to automatically change Babel config based on those TS settings, similar to how @wessberg/rollup-plugin-ts works. But I think it would be more explicit if TSDX users configured Babel directly instead to their preferences (no implicit guess work), so logging warnings may be more optimal. It's also less work, which is good given how little time I can have at any point.
  2. Investigate turning preset-env loose to false (or unset, rather) but leaving class-properties loose as true, same as CRA. Perhaps that solves this downlevelIteration issue without much impact. Would have to investigate perf impact, but that really should be secondary to correctness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: duplicate This issue or pull request already exists solution: workaround available There is a workaround available for this issue topic: downlevelIteration Related to tsconfig downlevelIteration
Projects
None yet
Development

No branches or pull requests

2 participants