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

fix: workaround for the features option schematics error #18923

Closed
wants to merge 7 commits into from

Conversation

kpawelczak
Copy link
Contributor

closes: CXSPA-7456

This pull request addresses the issue detailed in Angular CLI GitHub issue #16320. The problem is that using ng add with options of type array may not function correctly in some cases, and might only work on the second attempt.

To resolve this, we have implemented a workaround in the form of a new utility function, getFeaturesOptions. This function ensures that feature options are handled correctly, mitigating the problem described in the issue.

Changes:

  • Added getFeaturesOptions function to work around the issue with ng add and array options.
  • Changed the features type in schema.json

@kpawelczak kpawelczak requested review from a team as code owners May 31, 2024 14:22
@github-actions github-actions bot marked this pull request as draft May 31, 2024 14:22
Comment on lines 495 to 499
export function getFeaturesOptions<OPTIONS extends LibraryOptions>(
options: OPTIONS
): string[] {
return getArrayOptions(options.features ?? []);
}
Copy link
Contributor

@Platonn Platonn May 31, 2024

Choose a reason for hiding this comment

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

nitpick: Did you consider having a different signature of this util function?
Now we use it as:
options.features = getFeaturesOptions(options);, where getFeaturesOptions is a pure function that knows about the shape of OPTIONS

IMHO more semantic would be using it as:
a) options.features = convertToArray(options.features), where convertToArray is a pure function, which doesn't know about the shape of the OPTIONS object
OR
b) normalizeOptionsFeatures(options) which under the hood mutates the options.features property

Personally, I tend toward the option a)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I went with option B as I think it will work better with JSDoc.

@kpawelczak kpawelczak marked this pull request as ready for review June 3, 2024 09:15
Comment on lines +505 to +506
} else if (values.includes(',')) {
optionsArray = values.split(',');
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: shall we somehow trim whitespaces, if using as --features="Cart, Order, Checkout" (with spaces)?

  1. we should test it manually, if it works now (wihtout trimming)
  2. if it doesn't work with spaces, then let's fix it + add unit tests for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial thought was that the correct command is --features=Cart,Order,Checkout, as this is the syntax used for an array-type option. The command you provided assumes that the feature is a string. However, the type array | string is just a workaround. I believe we should still treat it as an array.

I haven't tested the whitespace theory yet.

@github-actions github-actions bot marked this pull request as draft June 3, 2024 11:16
@kpawelczak
Copy link
Contributor Author

After a review and testing, we found that this workaround is not viable as it disrupts the prompts

@kpawelczak kpawelczak closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants