ember-cli-typescript icon indicating copy to clipboard operation
ember-cli-typescript copied to clipboard

TypeScript hangs with Promise.all(array) (With ArrayPrototypeExtensions)

Open mogstad opened this issue 6 years ago • 22 comments

Which package(s) does this problem pertain to?

  • [ ] @types/ember
  • [ ] @types/ember__string
  • [ ] @types/ember__polyfills
  • [x] @types/ember__object
  • [ ] @types/ember__utils
  • [x] @types/ember__array
  • [ ] @types/ember__engine
  • [ ] @types/ember__debug
  • [ ] @types/ember__runloop
  • [ ] @types/ember__error
  • [ ] @types/ember__controller
  • [ ] @types/ember__component
  • [ ] @types/ember__routing
  • [ ] @types/ember__application
  • [ ] @types/ember__test
  • [ ] @types/ember__service
  • [ ] @types/ember-data
  • [ ] @types/rsvp
  • [ ] @types/ember-test-helpers
  • [ ] @types/ember-testing-helpers
  • [ ] Other
  • [ ] I don't know

What are instructions we can follow to reproduce the issue?

A small repo demonstrating a slow-down combining Promise.all while having interface Array<T> extends Ember.ArrayPrototypeExtensions<T> {}. Created it with as few moving parts as possible, so there is basically no code nor an ember app.

git clone [email protected]:mogstad/typescript-ember-slow-example.git
cd typescript-ember-slow-example
npm install
npm start

Now about that bug. What did you expect to see?

I expected the TypeScript code to be compiled swiftly.

What happened instead?

Running it on TS 3.0.3 takes 45 seconds, and 3.1.1 takes 8 minutes. We're only seeing this issue using @types/ember >= 3.

Workarounds

We've found a few workarounds, by either defining the generic type of Promise.all<T> (and its friends). (Same issue if using rsvp)

let items = await Promise.all<string>(promises);

Or only extending MutableArray<T> instead of ArrayPrototypeExtensions, we're not using anything in the Observable interface.

mogstad avatar Oct 09 '18 15:10 mogstad

@dwickern or @mike-north do you have any idea what's going on here? It's obviously an upstream issue in TS, but I'm also curious if we can at least identify why it's an issue so we can give them a good and useful report.

chriskrycho avatar Oct 10 '18 16:10 chriskrycho

Ran into this issue as well, it's pretty frustrating haha, since there is no messaging and the build just hangs.

knownasilya avatar Oct 24 '18 13:10 knownasilya

Same here. Would label this as a bug, as it makes development unbearable.

Commenting out the line interface Array<T> extends Ember.ArrayPrototypeExtensions<T> {} in types/package-name/index.d.ts "fixes" the problem, though it leads to other problems obviously...

simonihmig avatar Jan 09 '19 14:01 simonihmig

On my to-do list for most likely next week to get a solid repro so I can help the TS team chase down the root of this regression.

chriskrycho avatar Jan 09 '19 14:01 chriskrycho

I did some playing around with this over the holiday break. It seems to be related to the conditional types in UnwrapComputedPropertyGetter. It does not seem to be the infer (I took it out and same result) but when I change the conditionals to hard-coded types, it compiles fast.

Default types:

$ time tsc

real    0m44.633s

Remove infer (e.g. hard-code string instead of inferred generic U):

$ time tsc

real    0m46.230s

Remove conditional (e.g. type UnwrapComputedPropertyGetter<T> = string;):

$ time tsc

real    0m3.670s

I also found that compile time is dependent on the number of usages of UnwrapComputedPropertyGetter. If I start removing usages in @types/ember__object/observable.d.ts:

Remove get:

$ time tsc

real    0m23.995s

Also remove getWithDefault:

$ time tsc

real    0m9.545s

Also remove cacheFor:

$ time tsc

real    0m3.977s

Interestingly, removing getProperties (which uses UnwrapComputedPropertyGetters, which uses UnwrapComputedPropertyGetter) had very little affect on compile time:

Remove getProperties:

$ time tsc

real    0m42.193s

jamescdavis avatar Jan 09 '19 16:01 jamescdavis

That tracks with my own observation around where the regression happened in TS 3.1: it seems to be a regression when combining mapped types and conditional types.

chriskrycho avatar Jan 09 '19 16:01 chriskrycho

We worked around this in ember-osf-web (as @mogstad mentioned, but also including Copyable).

jamescdavis avatar Jan 09 '19 16:01 jamescdavis

3.1 definitely made it much worse:

$ npm install typescript@~3.1
+ [email protected]

$ time npx tsc

real    7m53.560s

but all those times above were with TS 2.8!

jamescdavis avatar Jan 09 '19 16:01 jamescdavis

@danielrosenwasser just raising this for your info; I'm going to try to have a minimal repro of some sort next week filed as a bug on the TS repo on both the basic compile time issues and the major 3.1 regression; any pointers as to the best way to get the info you all need for that would be 💯.

chriskrycho avatar Jan 09 '19 17:01 chriskrycho

Trying to spend a bit of time digging into this today. Will report what I find!

chriskrycho avatar Jan 30 '19 16:01 chriskrycho

Minimal repro 👍

But also reporting with --extendedDiagnostics would go farther.

DanielRosenwasser avatar Jan 30 '19 21:01 DanielRosenwasser

@DanielRosenwasser will do! Anything else I should look for in terms of isolating where it's coming from?

chriskrycho avatar Jan 30 '19 21:01 chriskrycho

Just keep deleting surrounding code until you get a reasonable build time and build back up - maybe try to see what array extensions trigger it. @sanders_n might know more.

DanielRosenwasser avatar Jan 30 '19 22:01 DanielRosenwasser

Ok, this is bizarre! Apparently, order matters. I'll explain.

For https://github.com/mogstad/typescript-ember-slow-example

Using tsc 3.2.4, I get:

$ time npx tsc

real    9m57.583s

If I go into node_modules/@types/ember__object/observable.d.ts and move the definitions for getProperties and setProperties (that use the mapped types UnwrapComputedPropertyGetters and UnwrapComputedPropertySetters) to be the last things in the interface (so that they are defined after everything that uses UnwrapComputedPropertyGetter and UnwrapComputedPropertySetter), I get:

$ time npx tsc

real    0m53.262s

I discovered this accidentally while creating a minimal repro (which I'll post shortly).

jamescdavis avatar Jan 31 '19 16:01 jamescdavis

That would explain some of the difficulty I've been having rebuilding an example that explodes. It also maps with my observation that imports seem to be relevant, likely because of resolution order?

chriskrycho avatar Jan 31 '19 16:01 chriskrycho

@DanielRosenwasser I've pushed a repo with a repro-of-sorts here. I'm happy to open an issue, but I'm actually not exactly sure how to capture this at this point:

  • there's a baseline of not great performance on this specific combination of features
  • there's a serious regression in the 3.1.x–3.2.x era (in this repo, ~5×) – maybe also 3.3.x, it was still present on 3.3.0-dev but I'm not sure about the current state
  • the performance regression appears to be fixed in 3.4.0-dev.20190131
  • but whatever changed so that the performance regression is resolved also broke a conditional type which worked from 2.8 forward

😬

chriskrycho avatar Jan 31 '19 20:01 chriskrycho

I've created a minimal repro. I found the same as @chriskrycho: major performance regression in 3.1-3.3 that appears to be fixed in 3.4, but there are other issues with conditional types. Those issues are not reproed in my performance regression repro, however.

jamescdavis avatar Feb 17 '19 21:02 jamescdavis

Would this only apply to .ts files in the project? My build seemed to be hanging and I narrowed it down to a Promise.all(ary) line, but it's in a .js file. It either permanently hangs the build, or triggers a build longer than I've ever been willing to wait.

I'm pretty new to typescript - does the compiler run on js files as well?

(I've replaced it with for (let p of ary) { await p; } as a workaround, which works fine)

courajs avatar Mar 16 '19 15:03 courajs

We came across the same problem luckily @mnunmununm from the discord channel pointed us to this issue. Hopefully, it can be resolved someday because it's not a good developer experience when you are blocked by this. Maybe we should add a section: known issues to the readme?

tschoartschi avatar Apr 08 '19 15:04 tschoartschi

@tschoartschi: The problem seems to be solved in typescript >=3.4.1

mschorsch avatar Apr 08 '19 15:04 mschorsch

@tschoartschi there's a Known Issues section in the docs, and I'd happily take a PR noting this is an issue on TS versions 3.1–3.3 there.

We also plan to address these kinds of issues over the next few months; there'll be an ember-cli-typescript RFC forthcoming outlining the strategy for that.

chriskrycho avatar Apr 08 '19 15:04 chriskrycho

@chriskrycho you mean the https://typed-ember.github.io/ember-cli-typescript/versions/master/docs/ts-guide/current-limitations section? I could write something explaining this issue. Or maybe it's enough to just link here. To be honest, I was quite lost today and spent the whole afternoon digging into why my build was broken.

@mschorsch thanks for the pointer to TypeScript 3.4.1. I'll need to check that our tomorrow :-)

Nevertheless, it's awesome work you guys did with ember-cli-typescript!

tschoartschi avatar Apr 08 '19 16:04 tschoartschi

Closing as resolved in TS 3.4. The learnings from this fed into our design of https://www.semver-ts.org and Ember's own policy that it is not required to add any given TS version to its support matrix, as well as our ecosystem-wide policy of testing against upcoming versions of TS to catch issues like this and report it upstream!

chriskrycho avatar Sep 28 '23 22:09 chriskrycho