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

virtualise engine styles with their vendor styles #1913

Merged
merged 2 commits into from
May 16, 2024
Merged

Conversation

mansona
Copy link
Member

@mansona mansona commented May 10, 2024

For #1779 it would be a lot easier for the implementation of an engine's own styles to be handled with the same virtual handler that deals with its vendor files. This PR makes that change 👍

@mansona mansona added the enhancement New feature or request label May 10, 2024
@mansona mansona force-pushed the virtual-compat-styles branch from 644e72e to 0360a74 Compare May 10, 2024 17:02
let result: string[] = impliedAddonVendorStyles(engine).map((sourcePath: string): string => {
let source = readFileSync(sourcePath);
return `${source}`;
});

// add engine's own implicit styles after all vendor styles
let styles = getAddonImplicitStyles(engine.package as V2AddonPackage);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not what I had in mind. Consider the app: the app never has implicit styles, but it does have it's own styles (the stuff from app/styles/app.css). That's the stuff we're talking about incorporating here.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but during the v2 upgrade of an app don't their styles get applied to the engine's implicit styles? I would have expected a lot more tests to fail in this PR if the approach didn't work 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess not 🙈 it turns out that the only test that checks for the app scripts right now is the namespaced app that has a qunit test Acceptance | smoke-test: styles present 🫠

I'll take another look at it

Copy link
Member Author

Choose a reason for hiding this comment

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

So I've implemented this now in such a way that it goes to find the right file out of the @embroider/synthesized-styles package and read its contents, but I'm wondering if we need to even generate a synthesised styles package any more 🤔

Looking at where it is generated https://github.com/embroider-build/embroider/blob/virtual-compat-styles/packages/compat/src/compat-app.ts#L505-L539 it seems to imply that there can be multiple files output other than just the app-name.css even though only one of the files was ever being consumed by the rewritten app. Am I missing some context here as to why it was written this way?

@mansona mansona changed the title virtualise engine files with their vendor styles virtualise engine styles with their vendor styles May 11, 2024
@mansona mansona force-pushed the virtual-compat-styles branch 3 times, most recently from 5b0aef2 to 97d4334 Compare May 15, 2024 16:26
@mansona mansona force-pushed the virtual-compat-styles branch from 97d4334 to 0a5d27b Compare May 15, 2024 16:26
@mansona mansona force-pushed the virtual-compat-styles branch from 0a5d27b to 0892a3a Compare May 15, 2024 16:40
@mansona
Copy link
Member Author

mansona commented May 15, 2024

I have now verified that this is working with the latest HEAD of #1779 👍

@ef4 ef4 merged commit 51043f4 into main May 16, 2024
95 checks passed
@ef4 ef4 deleted the virtual-compat-styles branch May 16, 2024 11:32
@github-actions github-actions bot mentioned this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants