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

In case incremental: false method rebuild should work as build instead being undefined #1394

Closed
AlexxNB opened this issue Jun 25, 2021 · 12 comments

Comments

@AlexxNB
Copy link

AlexxNB commented Jun 25, 2021

I have a config where I used incremental build, but then I need some tests with full rebuilding. In this case I rewrite all config to use build function instead rebuild method of result.

My suggestion is always to have rebuild method in result. And its behavior will depend on incremental property.

@evanw
Copy link
Owner

evanw commented Jun 25, 2021

That's an interesting suggestion. But why do you need some tests with full rebuilding?

@AlexxNB
Copy link
Author

AlexxNB commented Jun 25, 2021

This is a project with esbuild+svelte where was added sass later. Then I used @import for sass in the svelte's component and result css bundle didn't update when I changed something in sass files. That happened because svelte component wasn't changed and esbuild used cached result.
That is why I rewrite my esbuild config without incremental building. But I think that it will be very cool still use rebuild for rebuilding project with previously provided config options.

I use my own esbuild-svelte starter. See my defult config here: https://github.com/AlexxNB/svelte-esbuild-starter/blob/master/esbuild.js

@AlexxNB
Copy link
Author

AlexxNB commented Jun 25, 2021

This is not a critical feature. Just why not =)

@evanw
Copy link
Owner

evanw commented Jun 25, 2021

I'm asking because esbuild's incremental build logic shouldn't ever cause an incremental build to be different than a full build (assuming there aren't any buggy plugins), and I was wondering if this meant esbuild itself had a bug. But it seems like that's not the case here.

From what you described, it sounds like a plugin you're using has a broken cache implementation. You should reach out to the author of that plugin. The cache implementation should either be fixed or disabled. There is some advice about how to cache plugin transform functions here: https://esbuild.github.io/plugins/#caching-your-plugin.

That happened because svelte component wasn't changed and esbuild used cached result.

That's not something that esbuild does. Plugin output is never cached by esbuild. Plugins are always be re-run, even for incremental builds where the underlying files haven't changed. For example:

(async () => {
  const build = await require('esbuild').build({
    entryPoints: ['example.js'],
    incremental: true,
    plugins: [{
      name: 'example',
      setup(build) {
        build.onLoad({ filter: /.*/ }, args => {
          return { contents: `console.log(${Math.random()})` }
        })
      },
    }],
  })
  for (let i = 0; i < 3; i++) await build.rebuild()
  build.rebuild.dispose()
})()

This will generate a different build every time even though the content of example.js does not change.

The only difference between incremental builds and full builds is that some expensive internal tasks such as AST parsing are memoized. If a plugin returns exactly the same string for the same file as last time, the old AST is used instead of parsing a new one (which works because memoized ASTs are immutable). But that's irrelevant if a plugin returns a different string.

@AlexxNB
Copy link
Author

AlexxNB commented Jun 25, 2021

No. esbuild can't know that sass files changes. They are not imported by esbuild, it doesn't resolve them. Sass handles by svelte-preprocess and it doesn't watch for files which were imported in <style type="scss"> block.

@evanw
Copy link
Owner

evanw commented Jun 25, 2021

Sass handles by svelte-preprocess and it doesn't watch for files which were imported in <style type="scss"> block.

Does the plugin that does this have caching enabled? I'm guessing it does and the cache isn't being invalidated correctly. The cache needs to be invalidated if any of the imported files change, not just if the main entry point file changes. This is described here: https://esbuild.github.io/plugins/#caching-your-plugin. A simple way to check this is to see if disabling the cache fixes the problem. If so, the cache is broken.

If that's not the case (i.e. the plugin isn't doing any caching but incremental builds are still broken), then I'm happy to take a look and see if esbuild has a bug here. In that case the bug would be that esbuild somehow isn't re-running the plugin when it should be. I'd need a way for me to reproduce the issue myself to be able to fix it.

@AlexxNB
Copy link
Author

AlexxNB commented Jun 25, 2021

No. It is not a bug nor unexpected behavior. It was just a suggestion from me for better bundle-script syntax for some cases.

Sass-file that imported with @import directive inside custom <style type="scss">, handeled by svelte-preprocess - it is really edge case.

@evanw
Copy link
Owner

evanw commented Jun 25, 2021

I do consider it to be a bug. Incremental builds shouldn't ever generate different results from full builds. It looks like the bug is in the esbuild-svelte package so I filed it as EMH333/esbuild-svelte#59.

I'm not saying that your suggestion doesn't have merit; I think it's an interesting suggestion. But the reason why you wanted it in the first place indicates that something is wrong because that shouldn't be happening, so I wanted to try to fix the underlying issue too.

@Valexr
Copy link

Valexr commented Jun 26, 2021

it's not clear example, but in this project I'm using sass with esbuild-svelte plugin
incremental config with not updatable scss commented in esbuild.js file

@AlexxNB
Copy link
Author

AlexxNB commented Jun 26, 2021

@evanw Thank you for reporting to esbuild-svelte. If they can fix this, it will be cool.

@EMH333
Copy link

EMH333 commented Jul 3, 2021

As of esbuild-svelte v0.5.3 the underlying issue should now be fixed :)

@evanw
Copy link
Owner

evanw commented Jan 14, 2023

I'm closing this issue as the incremental API has been removed. Incremental builds now work differently as of version 0.17.0.

@evanw evanw closed this as not planned Won't fix, can't repro, duplicate, stale Jan 14, 2023
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

4 participants