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

For editing experience, use source instead of .d.ts files from project references #32028

Merged
merged 33 commits into from
Sep 24, 2019

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Jun 21, 2019

Instead of using .d.ts files from project references, use their original source files to give better editing experience.
I have played with this on our own compiler sources (eg. adding methods to public api of compiler and getting it reflected in services without having to build .d.ts file and also tried sample module scenario to see same behavior.

I would like to get the feel of the experience before actually working on remaining to dos

Things yet to look into:

  • errors in the project -> Skip type checking (so error reporting) on sources of project reference redirect
  • compile on save -> do not emit files that are sources of project reference redirects,
  • compile on save -> add error if compileOnSave is enabled but referenced projects don't have compileOnSave - Created Handle CompileOnSave on projects that contain project references. #33270
  • Fixing tests
  • Implications on memory and what can be done for exploding project size -> Add compile option to disable including sources of project reference redirect - option is disableSourceOfProjectReferenceRedirect

@sheetalkamat sheetalkamat changed the title [WIP] References prototype source file [WIP] For editing experience, use source instead of .d.ts files from project references Jun 21, 2019
src/server/project.ts Outdated Show resolved Hide resolved
@microsoft microsoft deleted a comment from typescript-bot Jun 21, 2019
@microsoft microsoft deleted a comment from typescript-bot Jun 21, 2019
@microsoft microsoft deleted a comment from typescript-bot Jun 21, 2019
@microsoft microsoft deleted a comment from typescript-bot Jun 21, 2019
@ajafff
Copy link
Contributor

ajafff commented Jun 23, 2019

I'm pretty excited about this. Editing experience is the only thing stopping me from using project references.

Here are some edge cases that might need to be handled:

  • Does this work with different compilerOptions in the referenced project (strictNullChecks, noImplicitAny, moduleResolution, paths, ...)?
  • Will autocomplete show //@internal symbols from the referenced project because they are not yet stripped out?
  • Assignability might be different from batch compilation when types of private properties are removed.

@sheetalkamat sheetalkamat changed the base branch from master to noDtsWhenNotUsingProjectReferences June 27, 2019 20:14
@microsoft microsoft deleted a comment from typescript-bot Jun 27, 2019
@microsoft microsoft deleted a comment from typescript-bot Jun 27, 2019
@AnyhowStep
Copy link
Contributor

AnyhowStep commented Jun 27, 2019

I have a question.

Is this a thing for composite projects?


If so, I think it's possible for your editor to say that you've reached the max instantiation depth, but when building, it'll build fine.

Especially if the editor is looking at source files and compiler is looking at .d.ts files.

The editor would have to "go deeper" to resolve types, while the compiler would not.

It would also break my current workaround for avoiding the max instantiation depth,
#29511 (comment)


If whatever I just said isn't relevant at all.. Just ignore me =x

@sheetalkamat sheetalkamat changed the base branch from noDtsWhenNotUsingProjectReferences to master June 27, 2019 21:02
@sheetalkamat
Copy link
Member Author

@Andarist When creating program if file to be included in program is ".d.ts" from project reference output, we include its input file. We know the mapping because composite projects need to list all files in the program so we have list of input and outputfiles mapping.

@Andarist
Copy link
Contributor

So this still requires generating .d.ts files on the disk?

@NoelAbrahams
Copy link

@sheetalkamat I've opened an issue to report a failure in error underlining:
#35587

This started happening after the switch to using source files as per this PR. I have captured some logs, although there is no 'Err' line as far as I can tell. But I the offending files are listed in the logs so that may help. Let me know where to mail them.

@VincentBailly
Copy link
Member

@sheetalkamat , we have been depending on this feature for a few months now and all is very nice and fast, thanks again. It seems like we have forgotten to update the wiki about Project References, it says that is work is still work in progress and that we need to check in .d.ts files.

@trusktr
Copy link
Contributor

trusktr commented Sep 21, 2020

Instead of using .d.ts files from project references, use their original source files to give better editing experience.

I'm not sure that's a good idea, because types from source are different than types from declarations, and they are not compatible in various ways. See here for a non-exhaustive list of issues: #35822

Something that could happen is, a new user is happy-go-lucky writing code, and it all works perfectly (because the editor says so). Then later they go and try to publish their code as a library (with declaration files) and it all breaks. It's a very bad experience (believe me from experience!).

@NoelAbrahams
Copy link

I'm not sure that's a good idea, because types from source are different than types from declarations, and they are not compatible in various ways.

@trusktr this is a perfectly good idea for the large number of companies writing in-house-only code.

Ideally, we don't want to work with .d.ts files at all.

We have found this implementation a huge help.

@trusktr
Copy link
Contributor

trusktr commented Nov 3, 2020

@NoelAbrahams I can see how that can be helpful if all sources are consumed directly (yay, class-factory mixins work in this case).

Using source files directly may lead to other issues, f.e.: #35744. The downside of this is that if any dependencies were compiled with different settings than the consumer project, then they may fail to be compiled in the consumer project. In your case, it may be fine, as I assume all your in-house projects all use the exact same tsconfig configuration.

It's just something people should be aware of, and unless those issues have been fixed, then in the general case the change of this PR isn't necessarily a final solution.

@trusktr
Copy link
Contributor

trusktr commented Nov 3, 2020

@sheetalkamat Wait a second, based on #40431 (comment) by @RyanCavanaugh, this PR compiles to .d.ts files in memory, then relies on those virtual declaration files?

If so, I think that can be the solution for #35744 if it isn't already.

@sheetalkamat
Copy link
Member Author

As mentioned in PR description this PR includes source files from the referenced project instead of generated .d.ts file. We do not generate in memory d.ts files.

@dko-slapdash

This comment has been minimized.

@andrewbranch
Copy link
Member

@dko-slapdash you posted the same question on three different issues/PRs (here, #39267 (comment), #34677 (comment)) within the course of an hour, before anyone had a chance to respond. That makes it incredibly difficult for us, and for future users who might have similar questions, to keep track of the conversation. I’m going to mark these off-topic, except for the one here, to avoid confusion.

@shrinktofit
Copy link

shrinktofit commented Dec 12, 2020

Do we still need composite: true to be set?

@Jack-Works
Copy link
Contributor

Hi! I have a question about monorepo+project reference settings.

The example I have the folder structure below:

/packages/main
    - tsconfig.json
    - app.ts
    - node_modules/@project/utils (symlink to /packages/utils)
/packages/utils
    - tsconfig.json

These two projects are configured to use project references. main/tsconfig.json references ../utils.

My question is: Do I need to add "paths": { "@project/utils": "../utils" } in main/tsconfig.json to speed-up the compilation?

I worry about if TypeScript does not resolve @project/utils to its real position, it will treat node_modules/@project/utils as a normal .d.ts reference instead of another TS project and lose the benefit of incremental compiling. If TypeScript knows @project/utils is a symlink and gets its real position and found it is a project reference, it would be great.

Can @sheetalkamat help? Thanks!

@andrewbranch
Copy link
Member

As long as you don’t have preserveSymlinks on, we should realize the path through node_modules is a symlink and the paths entry is not necessary. You do need references though. My convention is to put the relative path to the realpath in references.

@Jack-Works
Copy link
Contributor

As long as you don’t have preserveSymlinks on, we should realize the path through node_modules is a symlink and the paths entry is not necessary. You do need references though. My convention is to put the relative path to the realpath in references.

Thanks, but I still have a question, what about the following case?

/packages/main
    - tsconfig.json ("reference": [{ "path": "../utils/src" }])
    - app.ts
    - node_modules/@project/utils (symlink to /packages/utils)
/packages/utils
    - package.json ("main": "./dist", "types": "./dist")
    - src/tsconfig.json ("outDir": "../dist")

In this case, even if TypeScript can resolve node_modules/@project/utils to it's real position /packages/utils, there is no /packages/utils/tsconfig.json. If it goes through the package.json, it will find the type definition at /packages/utils/dist so it still compiles. But can TypeScript know /packages/utils/dist actually generated by project /packages/utils/src/tsconfig.json and keep that track?

@andrewbranch
Copy link
Member

Yes, I think that should work. The reference to /packages/utils/src/tsconfig.json gets analyzed first so there’s up-front knowledge of its input and output files. Is something not working as you expect or is this just hypothetical?

@Jack-Works
Copy link
Contributor

Yes, I think that should work. The reference to /packages/utils/src/tsconfig.json gets analyzed first so there’s up-front knowledge of its input and output files. Is something not working as you expect or is this just hypothetical?

It's just hypothetical. tsc -b runs slow so I'm trying to figure out why. Thanks!

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

Successfully merging this pull request may close these issues.