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

Update to glimmer-scoped-css 0.7.0 #1721

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

backspace
Copy link
Contributor

This doesn’t work, there must be configuration missing or something as boxel-ui no longer has styles/global.css in dist, which causes the host build to fail.

But it does fix that components in boxel-ui can have images in their CSS that gets copied into dist.

@backspace backspace self-assigned this Oct 25, 2024
Copy link

github-actions bot commented Oct 26, 2024

Host Test Results

  1 files  ±    0  1 suites  ±0   15m 1s ⏱️ - 5m 22s
12 tests  - 700  0 ✔️  - 710  0 💤  - 2    6 +  6  6 🔥 +6 
12 runs   - 705  -6 ✔️  - 721  0 💤  - 2  12 +12  6 🔥 +6 

For more details on these failures and errors, see this check.

Results for commit 6f8976e. ± Comparison against base commit b0bbab5.

This pull request removes 706 and adds 6 tests. Note that renamed tests count towards both.
Chrome 131.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand with autoExecute flag is automatically executed by the bot, panel closed
Chrome 131.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand with autoExecute flag is automatically executed by the bot, panel open
Chrome 131.0 ‑ Acceptance | Commands tests: a command sent via SendAiAssistantMessageCommand without autoExecute flag is not automatically executed by the bot
Chrome 131.0 ‑ Acceptance | Commands tests: a scripted command can create a card, update it and show it
Chrome 131.0 ‑ Acceptance | Freestyle: smoke check
Chrome 131.0 ‑ Acceptance | basic tests: glimmer-scoped-css smoke test
Chrome 131.0 ‑ Acceptance | basic tests: visiting realm root
Chrome 131.0 ‑ Acceptance | code submode tests > multiple realms: first item in add file realm dropdown is the currently displayed realm
Chrome 131.0 ‑ Acceptance | code submode tests > single realm > with connection to test realm: code submode handles binary files
Chrome 131.0 ‑ Acceptance | code submode tests > single realm: Clicking card in search panel opens card JSON in editor
…
Chrome 131.0 ‑ Global error: Uncaught Error: Failed to fetch realm info for http://localhost:4201/testuser/personal/: 404 at http://localhost:7357/assets/chunk.c309c262c8ab954258a6.js, line 460885  While executing test: Acceptance | code submode tests > multiple realms: first item in add file realm dropdown is the currently displayed realm 
Chrome 131.0 ‑ Global error: Uncaught TypeError: Failed to fetch at http://localhost:7357/assets/chunk.0ddd49f3363c299eb605.js, line 460885  While executing test: Acceptance | code submode | create-file tests > when user has permissions to both test realms: filename is auto-populated from display name 
Chrome 131.0 ‑ Global error: Uncaught TypeError: Failed to fetch at http://localhost:7357/assets/chunk.4f293f9d635a1d974848.js, line 460885  While executing test: Acceptance | code submode | file-tree tests: can open files 
Chrome 131.0 ‑ Global error: Uncaught TypeError: Failed to fetch at http://localhost:7357/assets/chunk.6058aa4cb485b7c7d703.js, line 460885  While executing test: Acceptance | code submode | editor tests: allows fixing broken cards 
Chrome 131.0 ‑ Global error: Uncaught TypeError: Failed to fetch at http://localhost:7357/assets/chunk.664abfdc0928b3893299.js, line 460885  While executing test: Acceptance | code submode | inspector tests: inspector will show module definition in card inheritance panel 
Chrome 131.0 ‑ Global error: Uncaught TypeError: Failed to fetch at http://localhost:7357/assets/chunk.95bc08383398c63db8a1.js, line 460885  While executing test: Acceptance | basic tests: visiting realm root 

♻️ This comment has been updated with latest results.

@ef4
Copy link
Contributor

ef4 commented Dec 19, 2024

I pushed a commit here that I hope helps unblock things:

  1. Upgrade @embroider/addon-dev to the unstable channel. That is where the feature in keepAssets() we need is released right now.
  2. Drop rollup-plugin-import-css. That's not what we want while building a library. Presumably it was added in response to an error message, but that error was due to the lack of the aforementioned keepAssets() feature that makes it interoperate with other CSS-producing plugins.
  3. I fixed a couple unrelated build warnings where rollup didn't understand the way interior imports were being done via the package name.

The thing to know about keepAssets is that it handles files that appear in an import statement somewhere in the code. It ensures that both the import statement and the corresponding file are propagated to the build output. It doesn't do anything with files that nobody is importing. So when this PR does import './hello.png';, that's a clever workaround to trick keepAssets into including hello.png in the build output, so that url('./hello.png') in the CSS down below will probably work during the app build.

keepAssets does not currently look inside CSS files to recursively identify more assets that need to be kept. That would be a good feature to add. That would eliminate the need for this kind of workaround.

Alternatively, one can use the copy plugin to just directly copy various files through to dist. I used that here for the global.css, etc.

@backspace
Copy link
Contributor Author

Thanks, I did have the unstable version before, I removed it in b85c6b0 not realising the changes needed haven’t been included in a release yet. What’s a good way to track this?

I posted in the Ember Discord but in case you have an idea, the hello.png I put in the button component to test out this approach is being made invalid during the copying process, in dist it’s 110kb vs 61kb originally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants