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

Deep config loading for typescript #646

Closed
wants to merge 1 commit into from

Conversation

spion
Copy link

@spion spion commented Jan 24, 2018

Adds support for the "extends" property of typescript configs.

@codecov-io
Copy link

codecov-io commented Jan 24, 2018

Codecov Report

Merging #646 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #646      +/-   ##
==========================================
- Coverage   89.15%   89.12%   -0.04%     
==========================================
  Files          62       62              
  Lines        1991     2013      +22     
==========================================
+ Hits         1775     1794      +19     
- Misses        216      219       +3
Impacted Files Coverage Δ
src/assets/TypeScriptAsset.js 100% <100%> (ø) ⬆️
src/Logger.js 43.54% <0%> (-4.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 391e17f...2fb838d. Read the comment docs.

@spion
Copy link
Author

spion commented Jan 24, 2018

Rebased on latest master

@DorianGrey
Copy link

Just wondering: Typescript already has an "official" way to load the config file - the tsc CLI uses this as well. See the unit tests here:
https://github.com/Microsoft/TypeScript/blob/79a1240a1978044560e08470e86363ed90fc1909/src/harness/unittests/configurationExtension.ts#L131-L144

Why not use this API?

@spion
Copy link
Author

spion commented Jan 25, 2018

Sounds like a good idea, I'll look into it. Main worry was that parcel wont know which files to watch/track, but I'll check if the whole hierarchy can maybe be extracted and added to the watchlist.

@devongovett
Copy link
Member

Yep, we need to do our own resolution so that Parcel knows which files to watch

@DorianGrey
Copy link

Oh, I see - the API function only returns the config, but not which files have been traversed to build it. A bit unlikely for watching.

In that case, I'd argue that Object.assign does not perform a deep merge, but that would be required to correctly implement the extends clause.

E.g:
tsconfig.base.json:

{
  "compilerOptions": {
    "target": "es5",
    "module": "esnext"
  }
}

extended by
tsconfig.json:

{
  "extends": "./tsconfig.base",
  "compilerOptions": {
    "module": "commonjs"
  }
}

has to result in

{
  "compilerOptions": {
    "target": "es5",
    "module": "commonjs"
  }
}

Another issue would be that - as you see in the examples above - it is not required to explicitly provide the file extension of the extended file. This is also described in the official docs in the Configuration inheritance with extends section.

@spion spion force-pushed the typescript-extend-config branch 2 times, most recently from 6ed7c1d to 3bd66f6 Compare February 4, 2018 17:37
@spion
Copy link
Author

spion commented Feb 4, 2018

So I managed to fix this by implementing a custom parse config host that tracks all the read files. I don't like how unclean the result ended up, but I didn't see any opportunities for improvements due to the strict non-async config host API.

Let me know if you see any opportunities to improve this. Also, let me know if you want me to remove that compiler host interface comment, I thought it might come handy if someone wants to make changes to the host.

Is there a way to write a test that determines whether the right files end up in the watchlist?

Adds support for the "extends" property of typescript configs.
@spion
Copy link
Author

spion commented Feb 5, 2018

Some concerns I have

  • This is going to get processed for every single TS file. Is that bad?
    • I'm not sure how costly recomputing the config is for every .ts file
    • If we cache the computation, not sure how to get notified when it changes (in watch mode) so we can invalidate the cache.

@DorianGrey
Copy link

The compiler host interface comment should be OK, since there is no other way to illustrate the API you're trying to adhere to.
I agree that this contains some more tricks, but I don't see a simpler way to properly intercept the file loading process performed by the typescript API.

This is going to get processed for every single TS file. Is that bad?

IIRC, an issue regarding bad transformation performance raised in ts-jest project after switching to the typescript API for loading the config, so - yes, it will quickly become quite bad, mostly because of the resulting IO. In the ts-jest project, a cache was added for the tsconfig to get around this, but that won't work here as long as the involved files are assumed to change after reading them for the first time.

@devongovett
Copy link
Member

Closing this due to inactivity. Perhaps we can come back to this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants