Add test for proxying non-js files during bundle #3203
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/6v3UK2fEFXdo9FWYRjYF2iZoY7sk |
| @@ -110,8 +110,6 @@ export async function createBuildState(commandOptions: CommandOptions): Promise< | |||
| const isSSR = !!config.buildOptions.ssr; | |||
| const isHMR = getIsHmrEnabled(config); | |||
|
|
|||
| // Seems like maybe we shouldn't be doing this... | |||
| config.buildOptions.resolveProxyImports = !config.optimize?.bundle; | |||
There was a problem hiding this comment.
What’s the reason for this change? This is a big change to bundled behavior.
There was a problem hiding this comment.
Oh—sorry. Just saw the linked issue / PR. Let me take a look at those.
There was a problem hiding this comment.
See comment here: #3194 (comment)
I’d like for this codechange NOT to land, because it‘s a breaking change. Reason our tests don’t catch it is because… well we just don’t have enough optimization tests. This is much riskier than our current test suite would indicate.
There was a problem hiding this comment.
Cool no worries, I was just writing a test for the change. Maybe we should look at increasing the coverage of the optimization tests generally in order to mitigate the chance of breakage in the future.
There was a problem hiding this comment.
Yeah I’d love to see that! And yeah as you pointed out in the PR, a file appearing that wasn’t there before was a hint that the output was very different. But in general, we have too few optimization tests. I added one recently using your new test setup and it wasn’t that hard to add (cssModules)! Once we have more of those, we can see how PRs like #3194 affect the output a lot better.
There was a problem hiding this comment.
Indeed! Alright well I will close this here then and can look at adding more optimization tests once I've wrapped my head around what it is supposed to do in all the various configurations.
Changes
Add test for proxying non-js files during bundle; bug raised here #3109 and PR #3194.
Testing
This is the test.
Docs
Test for bug fix only.