-
Notifications
You must be signed in to change notification settings - Fork 6.8k
build: add validation for prebuilt cdk css #15588
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
Conversation
*/ | ||
export function checkCdkPackage(packagePath: string): string[] { | ||
return ['overlay', 'a11y', 'text-field'] | ||
.filter(name => !existsSync(join(packagePath, `${name}-prebuilt.css`))) |
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.
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.
I'm not sure I follow the note about using a Glob. As for hardcoding the entry points, I don't think it's an issue, because we have a limited set of prebuilt styles that we're exposing.
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.
My concern is just that whenever we add a new style file to a CDK entry-point/or add a new entry-point w/ styles, we need to remember adding the entry-point name to that array.
For globs: I meant that we just look for any SCSS files in the format of src/cdk/{name}/_{name}.scss
. That way we could avoid the hard-coded array of entry-points that generate a prebuilt CSS file.
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.
That is a little brittle though. E.g. it won't handle the case where overlay
is generated, but a11y
or text-field
aren't.
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.
Maybe I don't follow what you are saying, but my stance is that:
- If there is a
{name}-prebuilt.scss
file, then it will be exposed at the root of the CDK - If there is a
_{name}.scss
file, then it will be exposed at the root of the CDK.
This is basically how the package-tools works: see here. I don't feel too strong about it, but it should be definitely possible to make the check follow our current package-tools expectation.
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.
My point was that the glob only verifies that some file with that pattern is generated, it won't verify that the exact files we're expecting are in the directory.
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.
Sorry if my comment was ambiguous. My point was that you could use the glob to identify the files in the source folder and then check if the corresponding output files exist in the release output.
Adds another check to the release validation script that ensures that the prebuilt CDK CSS has been created.
058ac73
to
b073cd8
Compare
I've reworked it based on the feedback @devversion. |
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.
LGTM
Adds another check to the release validation script that ensures that the prebuilt CDK CSS has been created.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds another check to the release validation script that ensures that the prebuilt CDK CSS has been created.