Fixed composes issue for embroider + v1 addon

salsify / ember-css-modules

CSS Modules for ambitious applications

MIT License

282 stars 50 forks source link

Fixed composes issue for embroider + v1 addon #279

Closed wondersloth closed 1 month ago

wondersloth commented 2 years ago

Summary

This fix addresses the issue demonstrated here: https://github.com/salsify/ember-css-modules/issues/278
This is the minimal change to address the problem.
In ModulesPreprocessor, if we normalize the inputTree to always have the ownerName (like moduleName) , will ensure addons generate hashes correctly between template and css files.

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.

dfreeman commented 2 years ago

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 commented 2 years ago

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:

https://github.com/salsify/ember-css-modules/blob/54f76710611d2c622039eba3eb9cdf6977293ac3/.github/workflows/ci.yml#L131-L134

wondersloth commented 2 years ago

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.