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

Replace use of parseJSON with JSON.parse #4517

Merged
merged 15 commits into from
Sep 14, 2019
Merged

Replace use of parseJSON with JSON.parse #4517

merged 15 commits into from
Sep 14, 2019

Conversation

danharrin
Copy link
Contributor

@danharrin danharrin commented Aug 4, 2019

This replaces the use of jQuery's parseJSON with its Vanilla alternative, JSON.parse.

@w20k
Copy link
Contributor

w20k commented Aug 4, 2019

Why exactly @danharrin? Micro optimization? :)

Minor note: PRs without description will be closed in future.

@danharrin danharrin changed the title Replace use of inArray with indexOf Replace use of jQuery with VanillaJS #1 Aug 4, 2019
@danharrin
Copy link
Contributor Author

danharrin commented Aug 4, 2019

Yup @w20k, I should be doing a few of these PRs. There is really no reason to be relying on jQuery in these situations when there are well-browser-supported Vanilla alternatives. As well as very minor optimisation improvements, these changes allow for better developer understanding of October's codebase, without prior knowledge of jQuery.

I've just added another fix for isArray to this PR.

@danharrin danharrin changed the title Replace use of jQuery with VanillaJS #1 Replace use of jQuery with VanillaJS (inArray and isArray) Aug 4, 2019
@w20k
Copy link
Contributor

w20k commented Aug 4, 2019

@danharrin add all the fixes or what you did in the description ;) Thanks!

@danharrin
Copy link
Contributor Author

@w20k all done :)

@danharrin
Copy link
Contributor Author

That fix is done, anything else @w20k?

@w20k
Copy link
Contributor

w20k commented Aug 4, 2019

@danharrin should be all ;) Will quickly test, after you're done, and ready for the next release 458.

@w20k w20k added this to the v1.0.459 milestone Aug 4, 2019
@danharrin
Copy link
Contributor Author

All done @w20k :) also just added some very simple parseJSON to JSON.parse replacements.

@danharrin danharrin changed the title Replace use of jQuery with VanillaJS (inArray and isArray) Replace use of jQuery with VanillaJS (inArray, isArray and parseJSON) Aug 4, 2019
@danharrin
Copy link
Contributor Author

Hey @w20k, any reason in particular why this was blocked?

@w20k
Copy link
Contributor

w20k commented Aug 5, 2019

@danharrin it's labeled blocked just to ensure it won't be merged or processed before the next build. Those are waiting for their turn :)

@LukeTowers
Copy link
Contributor

@danharrin what's the browser support for these changes? Specifically IE.

@danharrin
Copy link
Contributor Author

@LukeTowers
indexOf() and isArray() are both IE9+.
JSON.parse() is IE8+.

@LukeTowers
Copy link
Contributor

As well as very minor optimisation improvements, these changes allow for better developer understanding of October's codebase, without prior knowledge of jQuery.

I feel like these methods are more clear (except perhaps in the case of the JSON) as their jQuery counterparts instead.

@w20k
Copy link
Contributor

w20k commented Aug 11, 2019

Jquery under the hood is using those as well :) Pretty much, only null && undefined check is added.

@LukeTowers
Copy link
Contributor

@danharrin alright, can you change this to only be the parseJSON replacement?

@danharrin
Copy link
Contributor Author

Will sort out now @LukeTowers 👍

@danharrin
Copy link
Contributor Author

@LukeTowers done it sorry, got sidetracked 😆

@LukeTowers
Copy link
Contributor

@danharrin can you merge develop into your branch and run the compile again?

@danharrin
Copy link
Contributor Author

Sorted @LukeTowers 👍

@danharrin danharrin changed the title Replace use of jQuery with VanillaJS (inArray, isArray and parseJSON) Replace use of parseJSON with VanillaJS Aug 12, 2019
@danharrin danharrin changed the title Replace use of parseJSON with VanillaJS Replace use of parseJSON with JSON.parse Aug 12, 2019
@w20k
Copy link
Contributor

w20k commented Sep 11, 2019

@LukeTowers I guess it could be merged.

From jQuery Dev build 😄: jQuery.parseJSON = JSON.parse;
https://code.jquery.com/jquery-3.4.1.js

@LukeTowers LukeTowers modified the milestones: v1.0.459, v1.0.460 Sep 11, 2019
@LukeTowers LukeTowers merged commit 7b8feca into octobercms:develop Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants