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

Fixed composes issue for embroider + v1 addon #279

Closed

Conversation

wondersloth
Copy link

@wondersloth wondersloth commented May 11, 2022

Summary

Root Cause

  • embroider passes a different shaped tree, coming through ember-cli-preprocessor-registry that no longer scopes the tree with a moduleName.
  • test-packages/embroider-app was not broken because:
    • The app would build fine, without it's moduleName (not the same in classic build where it does provide moduleName in the path.

Suggestion

  • I think ModuleSourceFunnel could possibly be removed. If we were to normalize both the inputTree, and stylesTree.

@@ -0,0 +1,26 @@
import { module, only as test } from 'qunit';
Copy link
Author

Choose a reason for hiding this comment

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

Remove the only!

@dfreeman
Copy link
Member

This seems reasonable to me—thank you both for the fix and for the helpful writeup, @wondersloth!

It looks like the CI config needs a tweak now that one of the test packages has been renamed, but once that's fixed up and the only is removed, this looks good to go!

I think ModuleSourceFunnel could possibly be removed. If we were to normalize both the inputTree, and stylesTree.

That seems likely to me! Unfortunately I'm stretched pretty thin and what capacity I do have in this space is focused on charting a migration path to Embroider + v2 addon support, where most of the build work this addon does goes away and is instead handled by bundler plugins like css-loader, rollup-plugin-styles, etc.

Given that, I probably won't invest time myself in improvements like consolidating ModuleSourceFunnel in the near term, but I'd be happy to review a PR if it's something you want to take on 🙂

@rwjblue
Copy link

rwjblue commented May 16, 2022

It looks like the CI config needs a tweak now that one of the test packages has been renamed

Specifically, I think these need to be updated for the new package name:

- name: Octane Addon
run: yarn workspace octane-addon test
- name: Octane Addon With Custom moduleName
run: yarn workspace @my-namespace/octane-addon test

@wondersloth
Copy link
Author

wondersloth commented May 21, 2022

I've found the root cause for this issue in embroider. This fix can land as a bridge between those differences.

  • Update with a test demonstrating addon composition in an embroider app.

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.

3 participants