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

Remove the appuniversum SASS includePath #309

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

Windvis
Copy link
Contributor

@Windvis Windvis commented Aug 11, 2022

ember-appuniversum 1.3+ no longer uses the @appuniversum/appuniversum package so this config fails the build if the project updated to that version (and the host app doesn't install the package itself).

The config isn't needed on older ember-appuniversum versions either since those also add the includePath already.

ember-appuniversum 1.3+ no longer uses the `@appuniversum/appuniversum` package so this config fails the build if the project updated to that version.

The config isn't needed on older ember-appuniversum versions either since those also add the includePath already.
'node_modules/@appuniversum/appuniversum',
// This is needed for the editor plugins dummy app styles to work.
// They import the ember-rdfa-editor/a-dummy file, which imports the appuniversum styles
// with an absolute path which only works if this includePath is added.
'node_modules/@appuniversum/ember-appuniversum/app/styles'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems this is needed because the dummy apps import the ember-rdfa-editor/a-dummy file which imports from ember-appuniversum. In theory I think it should be possible to convert that to @import "../ember-appuniversum/a-settings"; but it seems ember-cli-sass still can't find the file then even though it's there on the "file system".

I assume it was added here so it didn't need to be added to each plugin's dummy app config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can in theory move ember-rdfa-editor/a-dummy up a level and import it like that, similar to how it works for the ember-rdfa-editor.scss file. Dummy apps would still need to be updated, but the config is no longer needed.

Copy link
Member

Choose a reason for hiding this comment

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

It was added because we were tired of broken builds and this seemed to fix it
I don't think anyone working on this project understands what is actually going on with sass imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, ember-cli-sass(?) seems to have issues with relative paths that refer to scss files of other addons (that are supposedly merged with the app/styles folder of the host app).

So the ember-rdfa-editor/_a-dummy.scss file can't import ember-appuniversum/something because that file doesn't exist in the ember-rdfa-editor folder, but ../ember-appuniversum/something doesn't work either for some reason. Moving that dummy file up a level makes the ember-appuniversum/something import work as well. I have no idea what the difference is. The includePath sidesteps the issue because ember-cli-sass also looks in the node_modules folder for the file.

In any case, since it's only used for the dummy apps of the editor (and plugins) I wouldn't ship that includePath to consuming apps since we might run into similar issues in the future, but changing it would break all the editor plugin dummy apps that depend on this being set.

@abeforgit abeforgit merged commit bfd5712 into development Aug 12, 2022
@abeforgit abeforgit deleted the chore/remove/appuniversum-includePath branch August 12, 2022 07:53
@abeforgit abeforgit added the bug Something isn't working label Aug 12, 2022
@Windvis
Copy link
Contributor Author

Windvis commented Aug 12, 2022

Thanks!

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.

2 participants