-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat(build): support @use or @import in component styles #16
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Thanks for the PR :)
I've one comment about documentation / implementation that conflict with each other.
To keep everything consistent, I think we should keep it relative to the project root.
I know this will be strange when the paths to include live outside the project, but we should have a way to configure that, similar to how Nx has {workspaceRoot}
identifiers that can be replaced.
That way, we can check if the string starts with this then resolve to the workspace root instead.
I'm going to set up some manner of integration testing before merging this too, so we can ensure that current use cases continue to work.
Once I have that integration testing set up, we can add a test case in this PR to ensure that this works as expected.
"type": "object", | ||
"properties": { | ||
"includePaths": { | ||
"description": "Paths to include. Paths will be resolved to project root.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mentions paths being resolved to project root, but the implementation in createConfig
is joining the workspaceRoot, not the project root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
I am not sure why the schema of stylePreprocessorOptions defined in angular/executors/webpack-browser.json is described relative to project root, but most of other paths are relative to workspace root.
Here is the one I should copy from angular/executors/browser-esbuild.json
"stylePreprocessorOptions": {
"description": "Options to pass to style preprocessors.",
"type": "object",
"properties": {
"includePaths": {
"description": "Paths to include. Paths will be resolved to workspace root.",
"...
}
},
...
},
From my thought, it looks not strange to set the path outside the project. If we leverage and keep the consistence of the path design among other executors, it will benefit people to migrate from webspack or esbuild more easily.
Weather to change the path design from relative to project root to workspace root or not, I will fix my mistake of inconsistence first.
ae0c14d
to
46383dc
Compare
Hi, |
There are errors when building if my component styles contains @use.
I thing we need this option to help sass compile successfully.
https://nx.dev/nx-api/angular/executors/webpack-browser#stylepreprocessoroptions
https://nx.dev/nx-api/angular/executors/browser-esbuild#stylepreprocessoroptions