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

Chore: Remove an indirect dependency on jsonify #9444

Merged
merged 1 commit into from Nov 1, 2017
Merged

Chore: Remove an indirect dependency on jsonify #9444

merged 1 commit into from Nov 1, 2017

Conversation

realityking
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[X] Other, please explain:

This removes an indirect dependency on jsonify which is unlicensed and not needed in supported environments. Doesn't yet slim down the installed size as ajv has the same dependency. A PR in its repo is also open ajv-validator/ajv#579

What changes did you make? (Give an overview)

Replaced json-stable-stringify with json-stable-stringify-without-jsonify. The modules are identical except for the dependency on jsonify.

Is there anything you'd like reviewers to focus on?

An alternative would be to use nickyout/fast-stable-stringify. I chose not to do so because I'm not sure if eslint can guarantee that there's never a circular dependency.

@eslintbot
Copy link

Thanks for the pull request, @realityking! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

  • The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@eslintbot
Copy link

Thanks for the pull request, @realityking! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

Replace json-stable-stringify with json-stable-stringify-without-jsonify.

This removes an indirect dependency on jsonify which is unlicensed and not needed in supported environments.
@eslintbot
Copy link

LGTM

@realityking realityking changed the title Replace json-stable-stringify with json-stable-stringify-without-jsonify. Remove an indirect dependency on jsonify Oct 15, 2017
@realityking realityking changed the title Remove an indirect dependency on jsonify Chore: Remove an indirect dependency on jsonify Oct 15, 2017
@not-an-aardvark not-an-aardvark added the chore This change is not user-facing label Oct 16, 2017
Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

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

Thanks for PR. Looks reasonable to me. I just wonder if we should wait for the change to happen in ajv first, before merging this in?

@kaicataldo
Copy link
Member

It makes sense to me to wait until ajv is updated and we can do both at once, but I'd also be fine with merging this now as long as we have a way to track/remember that we want to update ajv as well.

@realityking
Copy link
Contributor Author

realityking commented Nov 1, 2017

@ilyavolodin @kaicataldo ajv 5.3.0 has been released. That version removes the dependency on json-stable-stringify.

@aladdin-add
Copy link
Member

@realityking thank for the comment! I created a PR( #9557 ) to upgrade ajv@5.3.0

@ilyavolodin ilyavolodin merged commit 1b606cd into eslint:master Nov 1, 2017
@ilyavolodin
Copy link
Member

Thanks for PR!

@realityking realityking deleted the remove-jsonify branch November 1, 2017 21:54
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 1, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants