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: ignore empty IDs #11

Closed
wants to merge 1 commit into from
Closed

fix: ignore empty IDs #11

wants to merge 1 commit into from

Conversation

joscha
Copy link
Contributor

@joscha joscha commented Nov 27, 2015

On empty IDs the selector generator would die with an exception, because document.querySelectorAll('#') is not valid.
I thought about adding a specific check for empty IDs, however there might be more edge cases I am not aware, so I just check for a thrown exception.

This PR adds a fix and a test that fails without the fix.

@moorage
Copy link

moorage commented Nov 29, 2015

+1

@joscha
Copy link
Contributor Author

joscha commented Dec 16, 2015

@fczbkk any chance to get this merged and released?

fczbkk added a commit that referenced this pull request Dec 16, 2015
@fczbkk
Copy link
Owner

fczbkk commented Dec 16, 2015

@joscha Sorry for the delay. I was busy and this slipped my mind.

When I started to fiddle with it, I found some more cases that need to be checked. So I did a bigger rewrite of ID validation mechanism. It should now correctly check not only for an empty ID, but also for ID that contains whitespace, except when sanitized ID should contain whitespace (e.g. when it contains a colon that needs to be escaped with a sequence containing a space).

Version v1.0.2 should fix all these cases.

Thank you.

@fczbkk fczbkk closed this Dec 16, 2015
@fczbkk fczbkk self-assigned this Dec 16, 2015
@fczbkk fczbkk added the bug label Dec 16, 2015
@joscha
Copy link
Contributor Author

joscha commented Dec 16, 2015

@fczbkk cool! shall I rebase that branch then so we at least add the test?

@joscha
Copy link
Contributor Author

joscha commented Dec 16, 2015

ah, just saw you added that case.

@joscha joscha deleted the empty-id branch December 16, 2015 09:51
@joscha
Copy link
Contributor Author

joscha commented Dec 16, 2015

@fczbkk could you plublish 1.0.2 to npm please?

@joscha joscha restored the empty-id branch December 16, 2015 10:13
@fczbkk
Copy link
Owner

fczbkk commented Dec 16, 2015

@joscha Thanks for reminding me. Published: https://www.npmjs.com/package/css-selector-generator

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

Successfully merging this pull request may close these issues.

3 participants