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

Angular: Do not use default for includePaths #17876

Merged
merged 1 commit into from Apr 6, 2022

Conversation

Coly010
Copy link
Contributor

@Coly010 Coly010 commented Apr 4, 2022

Issue: In Nx Workspaces, the CLI evaluates the schema.json file slightly differently to how Angular CLI evaluates it.

The Angular CLI will only look at the parent object's default value and return that when the option is not provided.

The Nx CLI will look at the parent object's childrens' default values and build out an object that matches those defaults, as would be expected.

This leads to erroneous behaviors in Nx Workspaces, as the stylePreprocessorOptions coming from the app's browser build target will get overridden by an object of the form:

{
  stylePreprocessorOptions: {
    includePaths: []
  }
}

What I did

I removed the default empty array form includePaths as this is the source of the erroneous behavior. It also does not need to have a default value as it is an optional property.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Apr 4, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit eadc4fd. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@mandarini
Copy link
Contributor

Nice!!!! :D :D @shilman do you think this modification makes sense? It's been causing some issues on our side! :)

@kylegach
Copy link
Collaborator

kylegach commented Apr 4, 2022

Hey, @kroeder — Can you take a look at this, please?

Copy link
Member

@kroeder kroeder left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR! 🙂

@Coly010
Copy link
Contributor Author

Coly010 commented Apr 4, 2022

@kroeder let me know if there is anything to be done with the failing tests. I'm not sure how they could have failed given I didn't touch the areas of the failing tests? (Vue, CRA?)

@kroeder
Copy link
Member

kroeder commented Apr 5, 2022

I'm not sure actually. I heard yesterday there are issues with the pipeline

Anything we can do about the failing steps @ndelangen @shilman?
Chromatic fails, but it generates a link and the demo works as expected

@shilman shilman changed the title fix(angular): do not use default for includePaths Angular: Do not use default for includePaths Apr 6, 2022
@shilman shilman added angular bug nx Prioritize Nx compatibility for angular patch:yes Bugfix & documentation PR that need to be picked to main branch labels Apr 6, 2022
@shilman
Copy link
Member

shilman commented Apr 6, 2022

@kroeder we're working on those CI failures, which are unrelated to this PR. Merging! Thanks everybody!

@shilman shilman merged commit 56f3914 into storybookjs:next Apr 6, 2022
@Coly010 Coly010 deleted the fix-angular-builder-for-nx branch April 6, 2022 12:34
@Coly010
Copy link
Contributor Author

Coly010 commented Apr 6, 2022

Thanks!!

@mandarini
Copy link
Contributor

Whoohooo!!! :D

@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 9, 2022
shilman added a commit that referenced this pull request Apr 9, 2022
Angular: Do not use default for includePaths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
angular bug nx Prioritize Nx compatibility for angular patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants