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 TypeError for spaceless condition in media-feature-name-value-allowed-list #5581

Merged
merged 4 commits into from Oct 12, 2021

Conversation

lachieh
Copy link
Contributor

@lachieh lachieh commented Oct 8, 2021

Which issue, if any, is this issue related to?

Closes #5577

Is there anything in the PR that needs further explanation?

Following the discussion in the issue, this code implements the changes requested there.

@lachieh
Copy link
Contributor Author

lachieh commented Oct 8, 2021

@ybiquitous @jeddy3 is this really as simple of a change that needs to happen here? Is there a test that can cover this?

@hudochenkov
Copy link
Member

@lachieh you could use an example from the issue and add it to the tests for the rule.

@ybiquitous
Copy link
Member

@lachieh

As @hudochenkov suggested, it would be helpful to have a test case. 😃

In addition, it would be helpful if you would improve our custom type for postcss-media-query-parser like this:

-		nodes: Child[];
+		nodes?: Child[];

@lachieh
Copy link
Contributor Author

lachieh commented Oct 9, 2021

Thank you, both. Test case will go in the accepts, right? Since it doesn't trigger the error, we're just checking that it doesn't bomb?

@ybiquitous
Copy link
Member

@lachieh Yes!

@jeddy3 jeddy3 changed the title update rule to exit early if parsing is not possible Fix TypeError for spaceless condition in media-feature-name-value-allowed-list Oct 9, 2021
@jeddy3
Copy link
Member

jeddy3 commented Oct 9, 2021

@lachieh Thanks for the pull request. If you haven't already seen it, there are steps in the CONTRIBUTING guide to run the test runner on just a particular rule. Once you've added a test case (and made @ybiquitous suggested change), we can trigger the workflow for you.

@lachieh
Copy link
Contributor Author

lachieh commented Oct 9, 2021

The definition has been updated and the test case added. Thanks!

@hudochenkov
Copy link
Member

@lachieh there is TypeScript error in some other rule, because types were changed. Could you fix it? Otherwise we can help to fix it.

@lachieh
Copy link
Contributor Author

lachieh commented Oct 9, 2021

@hudochenkov surely can! I'll be back at my desk in about 24 hours.

@ybiquitous
Copy link
Member

Oh, my suggested type change occurred the type error on lib/rules/no-duplicate-at-import-rules/index.js. 😅

It seem OK to fall back to [] as below:

 			// extract media queries if any
-			const media = mediaParser(valueParser.stringify(restParams))
-				.nodes.map((n) => n.value.replace(/\s/g, ''))
+			const media = (mediaParser(valueParser.stringify(restParams)).nodes || [])
+				.map((n) => n.value.replace(/\s/g, ''))
 				.filter((n) => n.length);

@lachieh
Copy link
Contributor Author

lachieh commented Oct 12, 2021

All done! Thanks everyone!

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@lachieh Thanks! LGTM 😄

@jeddy3 jeddy3 merged commit a413527 into stylelint:v14 Oct 12, 2021
@jeddy3
Copy link
Member

jeddy3 commented Oct 12, 2021

LGTM, thanks!

v14 changelog entry added:

  • Fixed: media-feature-name-value-allowed-list TypeError for spaceless condition (#5581).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants