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

[JaosnHsieh]

Current Behavior

$ tsdx create -> basic

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

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.

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]

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]

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 https://github.com/facebook/create-react-app/issues/4263 which suggests poor perf impact. PR https://github.com/facebook/create-react-app/pull/4248 points to a bug/issue with MobX's use of decorators https://github.com/mobxjs/mobx/issues/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.