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

Latest Embroider Issues #251

Closed
4 tasks done
Tracked by #1461
evoactivity opened this issue Jul 7, 2023 · 2 comments · Fixed by #252
Closed
4 tasks done
Tracked by #1461

Latest Embroider Issues #251

evoactivity opened this issue Jul 7, 2023 · 2 comments · Fixed by #252
Labels

Comments

@evoactivity
Copy link
Owner

evoactivity commented Jul 7, 2023

The latest release of embroider no longer works with my previous PR #238

We'll need to upgrade @embroider/test-setup to see the test failuers.

Not sure how reverting the change will affect people on older versions of Embroider.

I'm opening this issue to discuss and track the work needed.

context: https://discord.com/channels/480462759797063690/568935504288940056/1126880945455235092

  • Upgrade @embroider/test-setup
  • Revert Embroider #238
  • Update asset inlining strategy
  • No dynamic imports/requires

PR #252

@evoactivity evoactivity added the Bug label Jul 7, 2023
@evoactivity
Copy link
Owner Author

@void-mAlex added additional context on discord


the embroider fix would be to use the importSync syntax instead of require I think
the move to app only breaks a few expectations on the part of the addon like the presence of the file in the addon folder
a lot of these things just happened to work in classic builds for various reasons but they are not compatible with modern build tooling
https://github.com/embroider-build/embroider/blob/main/packages/macros/README.md#importsync

@evoactivity
Copy link
Owner Author

evoactivity commented Jul 8, 2023

So to sum up the changes made

  • Update related dependencies
  • Bump node version in workflows to 16 so fastboot recognises nullish coalescing operator
  • Output an object of functions keyed by the asset id as an index.js inside the inlinded dir
  • Import the generated index.js as an es module in the helper
  • Look up the assetId function and run it to import the related svg
  • Added test to ensure files with apostophes work

Other improvements to infra that could be made:

  • Now that ember-release, canary and beta are on v5 might be worthwhile adding try scenarios for v4 LTS releases.
  • The symbol injector seems flaky in tests, I added a waitFor with a 10 second timeout but it still times out randomly.
  • Running ember try locally sometimes works and sometimes complains about @glimmer/validator being missing

but these can be done in another issue/pr.

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

Successfully merging a pull request may close this issue.

1 participant