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

Fix co-located components regressions #1632

Merged
merged 4 commits into from
Oct 10, 2023

Conversation

chancancode
Copy link
Contributor

@chancancode chancancode commented Oct 8, 2023

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

@chancancode chancancode force-pushed the colocation-bug branch 2 times, most recently from 1d6ac26 to 1101287 Compare October 8, 2023 07:50
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.
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding so many tests!

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 chancancode changed the title Add tests for co-located components regressions (#1619) Fix co-located components regressions (#1619) Oct 8, 2023
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) {
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Oct 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common-tags does all this, yeah?
https://www.npmjs.com/package/common-tags
stripIndent?

@ef4 ef4 merged commit 157dd62 into embroider-build:main Oct 10, 2023
202 checks passed
@mansona mansona added the bug Something isn't working label Oct 31, 2023
@NullVoxPopuli NullVoxPopuli changed the title Fix co-located components regressions (#1619) Fix co-located components regressions Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants