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

[Robustness] stringify: avoid relying on a global undefined #427

Merged
merged 1 commit into from
Dec 27, 2021

Conversation

Connormiha
Copy link
Contributor

@Connormiha Connormiha commented Dec 27, 2021

I just removed needless(for these cases) typeof for checking for undefined

update: Added checking for typeof in parser.

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

These are not needless at all. undefined can be redefined in every scope except the global scope in ES5, and in ES3 - which this package supports - it can be modified in the global scope otherwise.

@Connormiha
Copy link
Contributor Author

@ljharb

These are not needless at all. undefined can be redefined in every scope except the global scope in ES5, and in ES3 - which this package supports - it can be modified in the global scope otherwise.

Yes, but there are many places with same check. For example https://github.com/ljharb/qs/blob/master/lib/stringify.js#L155 or https://github.com/ljharb/qs/blob/master/lib/stringify.js#L195

@ljharb
Copy link
Owner

ljharb commented Dec 27, 2021

In that case, those should be changed to use typeof instead :-) want to update the PR to do that?

@Connormiha
Copy link
Contributor Author

@ljharb typeof or void 0? Which one is preferable?

@Connormiha Connormiha changed the title [Refactor] simplify checking for undefined [Refactor] Add compatibility for ES3 Dec 27, 2021
@Connormiha
Copy link
Contributor Author

@ljharb updated.

@ljharb
Copy link
Owner

ljharb commented Dec 27, 2021

Definitely the typeof, because it has minimal runtime weight.

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks! Almost there.

lib/stringify.js Outdated Show resolved Hide resolved
lib/stringify.js Outdated Show resolved Hide resolved
@ljharb ljharb changed the title [Refactor] Add compatibility for ES3 [Robustness] stringify: avoid relying on a global undefined Dec 27, 2021
Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks!

@ljharb ljharb force-pushed the simplify-checking-undefinded branch from 5c1223e to 0a1d3e8 Compare December 27, 2021 21:51
@codecov
Copy link

codecov bot commented Dec 27, 2021

Codecov Report

Merging #427 (0a1d3e8) into master (408ff95) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #427   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files           8        8           
  Lines        1408     1408           
  Branches      172      172           
=======================================
  Hits         1406     1406           
  Misses          2        2           
Impacted Files Coverage Δ
lib/stringify.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 408ff95...0a1d3e8. Read the comment docs.

@ljharb ljharb merged commit 0a1d3e8 into ljharb:master Dec 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants