Watch mode watches for changes in package.json files used in resolution #44935
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
I'm not sure, honestly. I think it depends on if services are constructed using a watch system or not. |
| @@ -861,6 +871,11 @@ namespace ts { | |||
| getConfigFileParsingDiagnostics(config), | |||
| config.projectReferences | |||
| ); | |||
| state.lastCachedPackageJsonLookups.set(projectPath, state.moduleResolutionCache && map( | |||
There was a problem hiding this comment.
Because these package json lookups are calculated when program is built.. If the program was already built and then you change package.json file and invoke tsbuild, the change wont be picked by ts build and it will be reported as upto date.
Also note that ts build uses the infomation from tsconfig to derive if things can be upto date. So we never watch module resolution etc. So this seems one off.
ts build also doesnt mean its incremental build so storing this info in tsbuildinfo is not enough.
There was a problem hiding this comment.
Yep - I said as much in the OP of this PR - node_modules lookups aren't part of the file set calculated by build mode (seems like an oversight, but also super nontrivial to calculate), so we have no idea what files to add to the watch set till we've built a program once and gotten the full file set. Still, this helps pick up changes that happen any time after the first build, which helps a lot in --watch , if not --incremental.
|
|
||
| // package.json changes invalidate module resolution and can change the set of loaded files | ||
| // so if we witness a change to one, we have to do a full reload | ||
| reloadLevel = ConfigFileProgramReloadLevel.Full; |
There was a problem hiding this comment.
This seems incorrect.. Full Reload means read config file again to calculate file names.. Why do we need to do that for package.json changes.
Also changesAffectResolution seems very unoptimized way of saying resolve all modules again. Need smarter way to invalidate resolutions.. Esp when someone is not using the node12+ stuff.. looks like every npm install will be problematic? (it many times spans across multiple updates if its many packages)
There was a problem hiding this comment.
A package.json change can change what arbitrary module specifiers in the program resolve to (when main, exports, or typesVersions change) and which imports are valid in arbitrary files (when type changes), so, yeah, a package.json change can invalidate the whole program.
There was a problem hiding this comment.
And do note, only the exports and type fields are new - we failed to pickup main and typesVersions changes and invalidations from package.json files entirely before. A fix for that actually landed before any of the node12 stuff.
There was a problem hiding this comment.
But invalidating whole program is different from reading config file and getting initial set which is what ConfigFileProgramReloadLevel.Full means
There was a problem hiding this comment.
Also package.json main and typeversion use to be rare and we use to watch failed lookups and in most cases that would cover it.. otherwise the npm install would need to re-do module resolutions for whole program.. Wouldnt it be still same that package.json affects certain files for module resolutions and not all.. eg. changing node's package.json shouldnt need to do module resolution in file that doesn't use node stuff..
There was a problem hiding this comment.
package.json main is about as far from "rare" as you can get - every package has one. :P
You would be right that we probably don't need to watch them if moduleResolution isn't node/node12/nodenext though. Not that many people use TS without one of those set, since module: commonjs (and thus moduleResolution: node) is the norm. Except I believe @types resolution unconditionally uses node-style resolution anyway; so I think we actually need to watch the package.json documents in those folders unconditionally...
| @@ -1192,12 +1207,14 @@ namespace ts { | |||
| watchExtendedConfigFiles(state, projectPath, config); | |||
| watchWildCardDirectories(state, project, projectPath, config); | |||
| watchInputFiles(state, project, projectPath, config); | |||
| watchPackageJsonFiles(state, project, projectPath, config); | |||
There was a problem hiding this comment.
This is incorrect location for these? Because program is yet not created so this won't be correct list? And all the package.json watches will be deleted.
There was a problem hiding this comment.
How so? This is the only place we add tsbuild watchers, afaik. It's a known limitation that we don't have a full list on first build (or, more like, we have an empty list because we don't have a good way to calculate a list).
There was a problem hiding this comment.
Because even though project config file was parsed to calculate the new filenames, options etc, the program was not created. that happens later during building of project so this cache is old cache and doing this has no effect?
This fixes an issue I noticed while writing tests for cts/mts (and
type: module) support - when apackage.jsonchanged, we weren't noticing it in watch mode (and consequently weren't invaliding resolutions and rechecking programs) - the fix is somewhat substantial, so is now a separate PR, this. (In build mode, this fix is wholly non-incremental - we'll only pick up changes to package files after we've done a build involving them once, since we pull the data from the resolution cache - we could probably fix this by serializing the list of used package file resolutions for a file/project into the build info, but that seems involved).cc @andrewbranch since you said including package json files in the normal watched file set may affect/help package autoimports.