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 the path to the on-disk styles file for in-repo engines #709

Merged
merged 5 commits into from
Mar 4, 2021

Conversation

charlespierce
Copy link
Contributor

@charlespierce charlespierce commented Mar 4, 2021

Info

  • When using in-repo engines, the path generated to the engine's styles is malformed, resulting in an error like:
Can't resolve '../../../account-engine/account-engine/__COMPILED_STYLES__/account-engine.css' in '$TMPDIR/embroider/8840c8/assets/_engine_'
  • There appear to be two issues: First, by passing the filename into explicitRelative, the path has one too many ../ elements at the beginning (since it treats the file as a directory as well).
  • Second, assuming that the engine will be in a top-level directory with the name equal to it's package name is wrong, as in-repo engines are contained in lib/{package-name}.

Changes

  • Updated the implicit-styles code to call dirname on the underlying relativePath (stripping off the filename from the path)
  • Updated the implicit-styles code to use engine.appRelativePath, instead of hard-coding it to be the package name for the engine, making sure it includes the lib/ if necessary.

Tested

  • Ran a build with an in-repo engine to confirm that the styles can be located by Webpack.
  • Updated the test scenarios to include an in-repo addon and confirm that it builds and works as expected.

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 4, 2021

Can you add an in-repo engine with styles to the engines-host-app?

@charlespierce
Copy link
Contributor Author

Updated to include an in-repo addon in the engines-host-app project. Also manually confirmed that the test fails to build without the changes in this PR (for the reasons described).

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.

None yet

2 participants