Fix synthesized template-only components crash #1620
Closed
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
|
|
|
|
|
Thanks for the fix 🎉 you inspired me and @NullVoxPopuli to pair on finally implementing app-level watch-mode tests in #1624 It would be super awesome if we could demonstrate your fix by having a failing test that then starts to pass with your PR, what do you think? |
|
That would be great! |
|
@chancancode we finally ironed out the issues with #1624 (I'm done with windows 😭) so you can create the watch mode tests now 👍 Let me know if you have any questions about the setup 👍 |
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.
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
force-pushed
the
cleanup-synthesized-template-only-components
branch
from
a390c25
to
fdf7d4c
Compare
|
Oops I accidentally pushed everything to #1632 so I’ll just close this |
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.
This fixes
the first scenario detailed in#1619. See the individual commit messages for details.