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

Tsbuild watching packagejson is dependent on whether program was built in the invocation #48314

Open
sheetalkamat opened this issue Mar 17, 2022 · 4 comments · May be fixed by #48889
Open

Tsbuild watching packagejson is dependent on whether program was built in the invocation #48314

sheetalkamat opened this issue Mar 17, 2022 · 4 comments · May be fixed by #48889
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.

Comments

@sheetalkamat
Copy link
Member

sheetalkamat commented Mar 17, 2022

With tsc --build we watch input files, config files and output file timestamps to determine if we need to rebuild or not. In past we never checked for module resolution and related files and their timestamps and requiring users to either force build if all inputs and outputs are UpToDate with only change to module resolution related changed. Eg node_modules/@types/someType/packge.json say exports or imports map or types entry changes.

Recently in anticipation to nodenext resolution changes we added package.json also as part of list of files to check timestamps on. But since it cannot be derived without having program constructed, it meant that we only watch/check their timestamps when we construct program. This means we benefit from this only in watch scenario and that too if program was constructed.

So here are two questions:

  1. Do we really need to watch/timestamp check package.json as now it could change fields that affect the module resolution
  2. If we need to treat package.json can we use heuristics or do we need fields. Note that we could store this list in buildinfo but then non incremental programs will have the same issue as there wont be buildinfo to read from?
    Options for package.json detection:
    a. package.json next to your own tsconfig, and next to any config referenced in references section?
    b. @RyanCavanaugh suggested packagejson entry in the tsconfig
    c. references section allows packge.json but are we allowing tsconfig options in package.json then?

Originally posted by @sheetalkamat in #44935 (comment)

@sheetalkamat
Copy link
Member Author

cc @weswigham

@weswigham
Copy link
Member

Yeah, definitely something we'd like to fix; unfortunately the way build mode operates just doesn't lend itself to a way to easily do this without doing exactly what build mode was intending to avoid - parsing every file for (nonrelative) specifiers and resolving them. The existing logic of pulling the data from the program cache is probably also a worthwhile optimization for non-initial builds, even if/when we can find a way to get the list for an initial build.

@sheetalkamat
Copy link
Member Author

But it seems like somewhat of inconsistent experience.

if i did

tsc -b

some change to package.json

tsc -b => // This does not detect change..  and completes as is.. At which point only way to observe change is to do clean compile or delete tsbuildinfo. 
// If we are relying here on change to source file or some such to observe the change why not in watch..

The behavior is different vs

tsc -b -w which detects the change and does the build.

The file watchers are normally polling so they have more cost associated with them.
What's the probability of only changing packge.json vs directory update (like npm install which replaces the file) if that is the case, may be directory watcher is better which is what we use for failed lookups..

Having said that i strongly believe, ts build should watch things that are derivable by config file or some heuristic files that can be observed with or without program creation

@weswigham
Copy link
Member

The file watchers are normally polling so they have more cost associated with them.
What's the probability of only changing packge.json vs directory update (like npm install which replaces the file) if that is the case, may be directory watcher is better which is what we use for failed lookups..

So in a monorepo setting (which is a primary usecase for build mode, if I recall correctly) only a package file changing without the whole directory structure changing is super common (about as common as people refactoring their file structure or packages I suppose). Package files are essentially simplified source files - they literally have imports in them, people change them as they please (and in ever increasing volume with modern package file features). Outside of that, most node_modules folders are usually installed once and done, and rarely updated.

Do we not use directory watchers wherever possible already? I'd have assumed a single directory watcher in the most-root location relevant to us that we then filter is what we already tried to do if the host supported it. I guess I'm not familiar with the performance characteristics of various file watching strategies, however.

Having said that i strongly believe, ts build should watch things that are derivable by config file or some heuristic files that can be observed with or without program creation

Watching every possoble package file path in a recursive directory structure is certainly an option I suppose (would be cheap and easy with a directory watcher I imagine). Certainly derivable without program data, and then the packages actually used in program creation can refine that in watch mode to a smaller set of relevant locations.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Mar 28, 2022
@sheetalkamat sheetalkamat added the Fix Available A PR has been opened for this issue label May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants