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

Plugin onLoad called before onStart? #1967

Closed
jfirebaugh opened this issue Jan 27, 2022 · 6 comments
Closed

Plugin onLoad called before onStart? #1967

jfirebaugh opened this issue Jan 27, 2022 · 6 comments

Comments

@jfirebaugh
Copy link

jfirebaugh commented Jan 27, 2022

At Figma we have a plugin with the following form:

export const cssPlugin = {
  name: 'css-modules',
  setup(build) {
    let statCache
    build.onStart(() => {
      statCache = new cache.StatCache()
    })
    build.onLoad({ filter: /\.css$/ }, async (args) => {
      // uses `statCache`
    })
  }
}

Occasionally, our esbuild pipeline flakes with an error that includes the following:

This error came from the "onLoad" callback registered here

../esbuild/src/plugin-css.ts:124:10:
  124 │     build.onLoad({ filter: /\.css$/ }, async (args) => {
      ╵           ~~~~~~

After adding instrumentation, we found that the error happens because statCache is undefined. The code does not assign statCache anywhere other than in the onStart callback (i.e. it does not assign undefined to it sometime after onStart), so as far as I can tell, the only way this error could happen is if esbuild called this plugin's onLoad callback before it called the onStart callback. (Another piece of evidence that is consistent with this explanation is that this flake always occurs on the very first .css file that onLoad processes.)

Is it expected that onLoad can be called before onStart?

@evanw
Copy link
Owner

evanw commented Jan 27, 2022

Perhaps it's because onStart runs concurrently with onLoad? From the onStart documentation:

The on-start callback can be async and can return a promise. However, the build does not wait for the promise to be resolved before starting, so a slow on-start callback will not necessarily slow down the build. All start callbacks are also run concurrently, not consecutively. The returned promise is purely for error reporting, and matters when the on-start callback needs to do an asynchronous operation that may fail or may produce warnings. If your plugin needs to wait for an asynchronous task in your on-start callback to complete before any on-resolve or on-load callbacks are run, you will need to have your resolve or load callbacks block on that asynchronous task.

There isn't an async task in your onStart callback but esbuild still runs them in an async context so that they can return a promise, so each onStart still goes through the event loop. Perhaps the documentation could be more precise about exactly how this happens. But anyway I suspect that having onLoad wait for onStart to finish will fix your issue.

@jfirebaugh
Copy link
Author

Hmm, interesting. I was indeed unaware of that part of the documentation.

Given those semantics, I don't think it's possible to obtain the desired behavior for this plugin using callbacks. The aim is to reset state that has been invalidated if the filesystem changed between builds. If there are no guarantees that an onStart callback has even begun execution before the first execution of onLoad, then something like the following is susceptible to essentially the same issue:

  setup(build) {
    let lastOnStartPromise
    let statCache
    build.onStart(() => {
      lastOnStartPromise = (async () {
        statCache = new cache.StatCache()
        // reset other state
      })()
    })
    build.onLoad({ filter: /\.css$/ }, async (args) => {
      await lastOnStartPromise // might be undefined
      // use `statCache`
    })
  }

@jfirebaugh
Copy link
Author

I guess maybe the following would work?

  setup(build) {
    let statCache
    async function reset() {
      statCache = new StatCache()
    }
    let resetPromise = reset()
    build.onLoad({ filter: /\.css$/ }, async (args) => {
      await resetPromise
      // use `statCache`
    })
    build.onEnd(() => {
      resetPromise = reset()
    })
  }

@jfirebaugh
Copy link
Author

Yeah, seems like using onEnd instead of onStart will work fine. Thanks for your help!

@evanw
Copy link
Owner

evanw commented Jan 27, 2022

Sorry about the confusing semantics. My goals were to avoid running onStart in serial and to avoid plugins with slow onStart callbacks holding up the build. Maybe I should reconsider these semantics though.

@jfirebaugh
Copy link
Author

jfirebaugh commented Jan 27, 2022

If you want to retain those semantics but still provide a clean way for plugins to establish per-build state, one option that I thought of would be to introduce a property of OnLoadArgs whose value is a promise which resolves to the result of the onStart callback for the current build. Say the property was pluginBuildContext, then I could write the plugin like this:

export const cssPlugin = {
  name: 'css-modules',
  setup(build) {
    build.onStart(async () => {
      return {
        statCache: new cache.StatCache(),
        // ...
      }
    })
    build.onLoad({ filter: /\.css$/ }, async (args) => {
      const { statCache, ... } = await args.pluginBuildContext;
      // use `statCache`
    })
  }
}

To avoid plugins with slow onStart callbacks holding up the build, I think you'd want the value of this property to be a promise even if the onStart callback returned a value synchronously.

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

No branches or pull requests

2 participants