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

Support scheme url starting with emoji #408

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

caiofct
Copy link

@caiofct caiofct commented Jan 24, 2024

This PR is a fix for #399. It essentially allows a schema url to start with any char including an emoji.

@caiofct
Copy link
Author

caiofct commented Jan 24, 2024

@gregjacobs This is the new PR. Sorry about the trouble.

@caiofct
Copy link
Author

caiofct commented Jan 25, 2024

@gregjacobs Any chance you could take a look at this one soon? Really appreciate if we could merge this and get a new version out so we could use the new version in our apps. Let me know if you need anything from me.

@gregjacobs
Copy link
Owner

Hey @caiofct. Sorry for the delay.

Unfortunately I don't think this is the correct fix (although it works at the moment, unexpectedly 😄 ). I'm looking more into it now though.

We don't want to conceptually allow emojis to be valid scheme characters because a scheme (like http or https) would always be only ascii letters or numbers. I think instead, when we encounter an emoji, we want to skip over it and put the state machine back into the "no match" state. I'll play with it a bit.

Best,
Greg

@caiofct
Copy link
Author

caiofct commented Feb 26, 2024

Hey @gregjacobs any luck on this issue?

Hey @caiofct. Sorry for the delay.

Unfortunately I don't think this is the correct fix (although it works at the moment, unexpectedly 😄 ). I'm looking more into it now though.

We don't want to conceptually allow emojis to be valid scheme characters because a scheme (like http or https) would always be only ascii letters or numbers. I think instead, when we encounter an emoji, we want to skip over it and put the state machine back into the "no match" state. I'll play with it a bit.

Best, Greg

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