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 #407

Closed
wants to merge 4 commits into from

Conversation

caiofct
Copy link

@caiofct caiofct commented Jan 22, 2024

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

Let me know if I've missed anything here since that's my first PR.

@gregjacobs
Copy link
Owner

Hey @caiofct, great stuff! Let me take a closer look when I get home tonight and we should be able to get it merged 👍

@gregjacobs
Copy link
Owner

Hey, so your first commit was good. The following 3 commits: not so much :)

image

When you're submitting a PR, you generally don't want to change a project's setup, tools, etc., unless you've first discussed it with the project's author. So for instance, changing a project from using npm to using yarn is a nono. Just because you like yarn better doesn't mean that everyone likes yarn better :) (p.s. yarn used to be good, but then they did some weird stuff and it's really annoying to install now, especially behind a corporate network. Sticking with npm at this point is fine, or use pnpm which is even better. But for a small open-source library, everyone already has npm if they have Node so not forcing contributors to figure out how to install new tools is ideal.)

Same with removing husky and puppeteer. Husky is used to run prettier on the Git precommit hook to keep code formatting consistent. And puppeteer is used in some of the integration tests that interact with a browser, so its removal breaks those tests.

And finally, the dist/commonjs and dist/es2015 folders were .gitignore'd for a reason. They're not needed for npm publishing, and would otherwise just cause many conflicts when merging concurrent PRs.

Bottom line: remove the last 3 commits and we should be able to merge :)

@caiofct
Copy link
Author

caiofct commented Jan 24, 2024

Hey, so your first commit was good. The following 3 commits: not so much :)

image

When you're submitting a PR, you generally don't want to change a project's setup, tools, etc., unless you've first discussed it with the project's author. So for instance, changing a project from using npm to using yarn is a nono. Just because you like yarn better doesn't mean that everyone likes yarn better :) (p.s. yarn used to be good, but then they did some weird stuff and it's really annoying to install now, especially behind a corporate network. Sticking with npm at this point is fine, or use pnpm which is even better. But for a small open-source library, everyone already has npm if they have Node so not forcing contributors to figure out how to install new tools is ideal.)

Same with removing husky and puppeteer. Husky is used to run prettier on the Git precommit hook to keep code formatting consistent. And puppeteer is used in some of the integration tests that interact with a browser, so its removal breaks those tests.

And finally, the dist/commonjs and dist/es2015 folders were .gitignore'd for a reason. They're not needed for npm publishing, and would otherwise just cause many conflicts when merging concurrent PRs.

Bottom line: remove the last 3 commits and we should be able to merge :)

Sorry. I was doing some test locally and ended up also submitting it to this PR as well. I'll prepare another branch with only the first commit so we should be good to go.

@caiofct caiofct closed this Jan 24, 2024
This pull request was closed.
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