Skip to content

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