Skip to content

Angular: Include object configured styles #24768

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.

Sign up for GitHub

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

Merged
valentinpalkovic merged 1 commit into from
Nov 9, 2023
Merged

Angular: Include object configured styles #24768

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions code/frameworks/angular/src/server/angular-cli-webpack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ exports.getWebpackConfig = async (baseConfig, { builderOptions, builderContext }

// Options provided by user
...builderOptions,
styles: builderOptions.styles
?.map((style) => (typeof style === 'string' ? style : style.input))

@andriy-statsenko-lightit andriy-statsenko-lightit May 13, 2024 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@valentinpalkovic hi. I think there is a slight mistake here since first you use map to take the string, but the filter will always have a string. Now objects with "inject": false will not be included in the build as separate files, now they will always be in the main.css file

@valentinpalkovic valentinpalkovic May 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @andriy-statsenko-lightit

Thank you for pointing this out. You're right! Fix incoming: #27108

@andriy-statsenko-lightit andriy-statsenko-lightit May 13, 2024 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@valentinpalkovic Thanks for the answer. It’s also worth noting that even if you swap the filter and map, files with “inject”: false will not be included in the final build as separate files. 🤔

@valentinpalkovic valentinpalkovic May 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe I misunderstood the inject property in the past. Are you saying we shouldn't filter at all, but inject only means whether the CSS is bundled in or outputted as a separate file?

@andriy-statsenko-lightit andriy-statsenko-lightit May 13, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ok so inject: false just excludes the bundle from the injection to index.html file, but the file will remain in the folder. inject: true includes the file to index.html. Example:
image
As I understand it, the problem was that files with inject: true were not included in theindex.htmlfile or were not included in the main.css file (not knowing exactly how the storybook builder works). After this fix, they will now be in the main.css file, but files with inject: false will not be created at all in the build (dist) folder.

@andriy-statsenko-lightit andriy-statsenko-lightit May 13, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah, if there is an object, it works ok, but if there is a path as a string, then files with “inject”: false simply do not appear in dist folder. This is interesting
image

With string path:
image

@valentinpalkovic valentinpalkovic May 13, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But that's great. Then I can kind of revert the work of this PR and do an internal mapping of styles defined as strings to the object notation. For the user, nothing changes, but things seem to work then!

@andriy-statsenko-lightit andriy-statsenko-lightit May 13, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah, it sounds like a great idea 🙂

@valentinpalkovic valentinpalkovic May 15, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've put together a fix here: #27108

@andriy-statsenko-lightit Would you mind to test the following canary release and give me some feedback? 0.0.0-pr-27108-sha-2470133a

@andriy-statsenko-lightit andriy-statsenko-lightit May 17, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@valentinpalkovic yeah, it works fine with different styles arrays: string only, string + objects, and objects only. Thanks!

.filter((style) => typeof style === 'string' || style.inject !== false),

// Fixed options
optimization: false,
Expand Down