Skip to content

Serialize builds to prevent stale output race#4

Merged
christian-bromann merged 4 commits intostencil-community:mainfrom
timurzholudev:main
May 29, 2025
Merged

Serialize builds to prevent stale output race#4
christian-bromann merged 4 commits intostencil-community:mainfrom
timurzholudev:main

Conversation

@timurzholudev
Copy link
Copy Markdown
Contributor

Description:

The unplugin-stencil plugin does not update the dist folder after launching the build and modifying a component.

Changes Made

Fix race condition where transform() could return stale output if a new build started during an ongoing one. Builds are now serialized and awaited using a version-safe loop, with a debounce added to coalesce rapid file changes.

@timurzholudev
Copy link
Copy Markdown
Contributor Author

Note: performance is struggling; it takes around 3-4 seconds in watch mode. This was the only way I managed to fix the issue with the race condition. Any feedback or improvement suggestions would be great to hear ❤️

Copy link
Copy Markdown
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work on this. I tested it out and it works. Performance wise we would need to look into the Stencil compiler directly and allow to re-compiler single files rather than the whole project. I think we can gain some performance wins there.

I have a suggestion regarding the implementation: I think it makes sense to wrap the compiler usage into a specific class to avoid confusion in the code. This way we can keep the interaction with the Stencil compiler separate from the Unplugin. An initial implementation could look like this:

class BuildQueue {
  #buildId = 0
  #compiler: CoreCompiler.Compiler
  #queue: [number, Promise<CoreCompiler.CompilerBuildResults>][] = []

  constructor(compiler: CoreCompiler.Compiler) {
    this.#compiler = compiler
  }

  #schedule() {
    const buildId = ++this.#buildId
    this.#queue.push([
      buildId,
      this.#compiler.build()
    ])

    /**
     * only keep last 10 builds
     */
    if (this.#queue.length > 10) {
      this.#queue.shift()
    }

    return buildId
  }

  async waitForLatestBuild() {
    const [latestBuildId, latestBuild] = this.#queue[this.#queue.length - 1]
    await latestBuild
    return latestBuildId
  }

  async ensureFreshBuild(srcPath: string, distPath: string) {
    try {
      const [srcStats, distStats] = await Promise.all([
        compiler?.sys.stat(srcPath),
        compiler?.sys.stat(distPath),
      ])
  
      if (
        distStats?.mtimeMs
        && srcStats?.mtimeMs
        && distStats.mtimeMs >= srcStats.mtimeMs
      ) {
        return
      }
    }
    catch {}
  
    this.#schedule()
    await this.waitForLatestBuild()
  }
}

Wdyt?

@timurzholudev
Copy link
Copy Markdown
Contributor Author

Sure, sounds good! Gonna update pr

@timurzholudev
Copy link
Copy Markdown
Contributor Author

I have a suggestion regarding the implementation: I think it makes sense to wrap the compiler usage into a specific class to avoid confusion in the code. This way we can keep the interaction with the Stencil compiler separate from the Unplugin. An initial implementation could look like this:

I am trying to implement the suggested class, and the major works well. The only problem is saving while there is an ongoing build.

For example, you saved some change and straight away do quick change and save, at this moment I am getting

[ ERROR ]  unable to find item fileText to write:
           /Users/tim/git/unplugin-stencil/playground/dist/components/index.js at commitWriteFile
           (/Users/tim/git/unplugin-stencil/node_modules/.pnpm/@stencil+core@4.29.3/node_modules/@stencil/core/compiler/stencil.js:264512:13)
           at
           /Users/tim/git/unplugin-stencil/node_modules/.pnpm/@stencil+core@4.29.3/node_modules/@stencil/core/compiler/stencil.js:264504:16
           at Array.map (<anonymous>) at commitWriteFiles
           (/Users/tim/git/unplugin-stencil/node_modules/.pnpm/@stencil+core@4.29.3/node_modules/@stencil/core/compiler/stencil.js:264500:20)
           at Object.commit
           (/Users/tim/git/unplugin-stencil/node_modules/.pnpm/@stencil+core@4.29.3/node_modules/@stencil/core/compiler/stencil.js:264437:32)
           at async writeBuild
           (/Users/tim/git/unplugin-stencil/node_modules/.pnpm/@stencil+core@4.29.3/node_modules/@stencil/core/compiler/stencil.js:263323:27)
           at async build

@christian-bromann
Copy link
Copy Markdown
Member

This seems to come from the in-memory-fs of Stencil. I assume the fileText is null during the time the build runs. I would suggest the following: if a build is in process and the user saves the file, set a flag in the class to re-trigger the build once finished and don't build again. Once the running build finishes and the flag is enabled, trigger a new build and disable the flag again.

src/index.ts Outdated
Comment on lines +124 to +130
await buildQueue?.ensureFreshBuild(id, compilerFilePath)

const exists = await compiler.sys.access(compilerFilePath)
if (!exists)
throw new Error('Could not find the output file')

const raw = await compiler!.sys.readFile(compilerFilePath)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this into the build queue and rename ensureFreshBuild to something like `getLatestBuild(id, compilerPath)

Suggested change
await buildQueue?.ensureFreshBuild(id, compilerFilePath)
const exists = await compiler.sys.access(compilerFilePath)
if (!exists)
throw new Error('Could not find the output file')
const raw = await compiler!.sys.readFile(compilerFilePath)
const raw = await buildQueue?.getLatestBuild(id, compilerFilePath)

catch {}

this.#queueBuild()
while (this.#isBuilding || this.#pending) await new Promise(r => setTimeout(r, 25))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about having this class extend from EventEmitter and do something like this:

Suggested change
while (this.#isBuilding || this.#pending) await new Promise(r => setTimeout(r, 25))
return new Promise((resolve) => this.once('buildFinished', resolve))

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea!
Learned something new 😂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Copy Markdown
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, LGTM 👍

@christian-bromann christian-bromann merged commit 4a62e8f into stencil-community:main May 29, 2025
10 checks passed
@christian-bromann
Copy link
Copy Markdown
Member

@timurzholudev I had to push a little fix as I was testing the new version with the storybook plugin. In transform we can't return undefined as this would mean that we leave it up to Vite to compile the file which can't compile it correctly.

@timurzholudev
Copy link
Copy Markdown
Contributor Author

@timurzholudev I had to push a little fix as I was testing the new version with the storybook plugin. In transform we can't return undefined as this would mean that we leave it up to Vite to compile the file which can't compile it correctly.

Oh, I don't know why I didn't test new changes in the storybook plugin 🤦

Great catch,
Thank you for the update ❤️

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.

2 participants