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

Race condition interaction (in rollup watch) between rollup/plugin-typescript and plugins that generate typescript #1122

Closed
DillonSadofsky opened this issue Mar 2, 2022 · 4 comments

Comments

@DillonSadofsky
Copy link

  • Rollup Plugin Name: @rollup/plugin-typescript
  • Rollup Plugin Version: 8.3.1
  • Rollup Version: 2.68
  • Operating System (or Browser): Window 10
  • Node Version: 16.13.0
  • Link to reproduction (⚠️ read below): This issue is a race condition and therefore cannot be given a consistent minimal reproduction, but I have already debugged the lines in question which are outlined below, so please consider this ticket.

Expected Behavior

When running rollup in watch mode, if another rollup plugin runs before the typescript rollup plugin that generates a .ts file on disk as part of the load event callback, typescript should see that new version of the file.

Actual Behavior

If a rollup plugin generates a .ts file on disk as part of load before the typescript rollup plugin is called, an out of date version of the .ts file will be used. I was able to step through typescript code to see why and its because internally when in watch mode, typescript adds a 250ms delay between noticing a file change and actually rebuilding the files. Because of this, @rollup/typescript-plugin's cache variable emittedFiles still contains a 'previous' version of the file that was part of the previous run of rollup. This leads to situations where the output build.js contains stale versions of the file, and running it again causes it to update to the correct output.

I set breakpoints and verified that the other rollup plugin does complete its work synchronously and that the files on disk are updated before the load(id) function gets called in @rollup\plugin-typescript\dist\index.js, and I was able to set a breakpoint in the writeFile callback (line 748 of the same file) to see that this code was called by a 250ms delayed build operation kicked off by scheduleProgramUpdate() (line 121770 of typescript\lib\typescript.js), so the emittedFiles cache gets updated, but after the stale version has already been consumed.

Additional Information

Essentially, the narrative is this:

  1. run rollup in watch mode. In the initial build, all files are good, output is correct.
  2. Cause rollup to trigger an incremental build from watch, but not by changing a .ts file (so a file that will cause a .ts file to be generated)
  3. Have another rollup plugin synchronously generate a .ts file output as part of the load handler that the typescript plugin is 'watching'.
  4. Typescript notices this change, and schedules a rebuild with a 250ms delay in function scheduleProgramUpdate in typescript\lib\typescript.js line 121770
  5. rollup/typescript-plugin has load(id) called on the input file; it checks emittedFiles which has a previously cached version of the file (since the callback to update the cache with the newly built file will not be called until after a 250ms delay). This stale version is used.
  6. 250ms later, typescript compiles the files, and the callbacks cause the rollup/typescript-plugin cache in emittedFiles to update, but too late.

I think the delay inherent in function scheduleProgramUpdate() on line 121762 of typescript.js makes sense for debouncing when typescript is run directly via tsc -w, but when the watch is 'internal' to a rollup plugin, maybe it should be disabled? Alternatively, rollup/plugin-typescript could be modified to not rely on typescripts' internal watch to notice the change but dirty its cache itself and retrieve output synchronously during a build. See below load-bearing typescript/plugin code.

typescript.js offending code:

        // Upon detecting a file change, wait for 250ms and then perform a recompilation. This gives batch
        // operations (such as saving all modified files in an editor) a chance to complete before we kick
        // off a new compilation.
        function scheduleProgramUpdate() {
            if (!host.setTimeout || !host.clearTimeout) {
                return;
            }
            if (timerToUpdateProgram) {
                host.clearTimeout(timerToUpdateProgram);
            }
            writeLog("Scheduling update");
            timerToUpdateProgram = host.setTimeout(updateProgramWithWatchStatus, 250);
        }

which causes this callback to get called on a delay (from @rollup/plugin-typescript/dist/index.js line 748):

                    writeFile(fileName, data) {
                        if (parsedOptions.options.composite || parsedOptions.options.incremental) {
                            tsCache.cacheCode(fileName, data);
                        }
                        emittedFiles.set(fileName, data);
                    },

And since this code in load(id) on line 802 uses the in-memory cache emittedFiles which has not been updated yet, a stale version is returned:

const output = findTypescriptOutput(ts, parsedOptions, id, emittedFiles, tsCache);

If its illustrative, here is an excerpt of my rollup plugins showing order of these plugins (the first generates .ts files and then of course typescript consumes them):

		// Must be before typescript
		ractive({
			outputExtension: '.ts',
			autoExport: true,
			outputDir: './client/.views/',
		}),
		typescript({
			tsconfig: './client/tsconfig.json',
		}),
@stale stale bot added the x⁷ ⋅ stale label May 1, 2022
@stale
Copy link

stale bot commented May 2, 2022

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it.

@stale stale bot closed this as completed May 2, 2022
@mansona
Copy link

mansona commented Apr 24, 2023

Hey folks can we have someone look at this instead of it just going stale with the stalebot? This seems to be a well-described issue and even though I can't evaluate the specifics of it I would like to know the maintainers' thoughts on it.

We're dealing with some infinite rebuild issues when using @rollup/plugin-json and this issue is the only thing that is remotely relevant on the repo

@DillonSadofsky
Copy link
Author

Yeah, I've never really gotten any attention on this. My team has either learned to deal with double/triple builds when doing TS and rollup and/or some of our projects have started to migrate away from rollup.

I've never personally had an infinite build, just usually double build, but depending on your watch files maybe there could be a circular issue.

@luxaritas
Copy link

luxaritas commented Apr 25, 2023

For what it's worth, I've migrated to rollup-plugin-typescript2 and haven't had an issue

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

No branches or pull requests

3 participants