Fix co-located components regressions #1632
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chancancode
force-pushed
the
colocation-bug
branch
2 times, most recently
from
1d6ac26
to
1101287
Compare
These tests are currently written to exhibit the buggy behavior. When I fix the issue I'll adjust the tests to the correct/expected behavior.
chancancode
force-pushed
the
colocation-bug
branch
from
1101287
to
4c86089
Compare
NullVoxPopuli
approved these changes
Oct 8, 2023
This fixes the first scenario detailed in embroider-build#1619, where a synthesized template-only component .js file is not cleaned up when the corresponding .hbs file is deleted. There are actually two bugs here. The first issue is we never attempt to clean up these unneeded files, which is fixed by keeping track of which of the emitted files are still needed after the main loop. The second issue is that, even with that fix, it still does not work, because we are incorrectly using the async version of the fs API here without awaiting for the promise. With the way the promisify code is written, the fs delete is actually scheduled to happen on the microtask queue, so by the time the `mergeTrees` code runs, the deletion hasn't actually happened yet. Since the synthesized tree appears after the appTree, I believe (but haven't tested) this has also been causing a different bug, where if you started with just a `.hbs` file, then add a `.js` file later, the new file does not actually take effect until one more build later, since the build triggered by the `.js` file creation would get shadowed by the previously synthesized template-only component file. Perhaps no caught this issue because the typical workflow involves creating either an empty file or generating a stub `.js` file, then filling it in with the actual content. By the "first meaningful save", the deletion would probably have gone through and this would self-resolve.
As discussed in embroider-build#1619, tools like babel caches the ouput based on the source content of the input files. For component javascript files, whether there is a co-located template file is an extra bit of information that doesn't show up in the source file, but that information does get used in producing the output. This causes the caches to not invalidate when a co-located tempalte file is added or deleted. This fixes the problem by ensuring we include that information in the input source file. For now, it is just an inert comment, but we can actually adjust our babel plugin to rely on this information rather than doing its own filesystem probing again, which should have some performance benefit.
chancancode
changed the title
| await waitFor(/Build successful/); | ||
|
|
||
| // TODO: find a better way to test this; this seems to linger around | ||
| // assert.false(await checkScripts(/js$/, originalContent), 'the original file does not exists'); | ||
| assert.true(await checkScripts(/js$/, updatedContent), 'the updated file now exists'); | ||
| }); | ||
|
|
||
| Qmodule('[GH#1619] co-located components regressions', function (hooks) { |
There was a problem hiding this comment.
I really like this style of test naming - the GH reference gives future viewers additional context as to why a test was added
| } | ||
| } | ||
|
|
||
| function d(strings: TemplateStringsArray, ...values: unknown[]): string { |
There was a problem hiding this comment.
common-tags does all this, yeah?
https://www.npmjs.com/package/common-tags
stripIndent?
NullVoxPopuli
approved these changes
Oct 8, 2023
NullVoxPopuli
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These tests are currently written to exhibit the buggy behavior. When I fix the issue I'll adjust the tests to the correct/expected behavior. These tests should pass right now (since the expectations in the tests are for the buggy behavior), so this can be merged even before the fix is ready.see the individual commits and #1619 for details