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.

Sign up for GitHub

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

Jump to bottom

Only use own properties in query string serializer #105

Merged
merged 1 commit into from
May 6, 2019
Merged

Only use own properties in query string serializer #105

merged 1 commit into from
May 6, 2019

Conversation

EvanHahn
Copy link
Contributor

Before (URI parameters decoded for readability):

function Person(name) {
    this.name = name;
}

Person.prototype.species = 'human';

objectToQueryParamString({me: new Person('Cher')})
// => 'me[name]=Cher&me[species]=human'

After:

objectToQueryParamString({me: new Person('Cher')})
// => 'me[name]=Cher'

This adds test coverage for this helper.

ESLint is the main motivation for this change, where our existing code will throw a guard-for-in error.

Only use own properties in query string serializer
6210813 
Before (URI parameters decoded for readability):

    function Person(name) {
        this.name = name;
    }

    Person.prototype.species = 'human';

    objectToQueryParamString({me: new Person('Cher')})
    // => 'me[name]=Cher&me[species]=human'

After:

    objectToQueryParamString({me: new Person('Cher')})
    // => 'me[name]=Cher'

This adds test coverage for this helper.

ESLint is the main motivation for this change, where our existing code
will throw a [`guard-for-in` error][1].

[1]: https://eslint.org/docs/rules/guard-for-in
@EvanHahn EvanHahn requested review from jbbakst and kasrak April 30, 2019 19:18
@EvanHahn EvanHahn merged commit 3b9e644 into master May 6, 2019
@EvanHahn EvanHahn deleted the only_use_own_properties_in_query_string_serializer branch May 6, 2019 15:24
EvanHahn pushed a commit that referenced this pull request Jul 9, 2019
Switch from JSHint to ESLint
9861673 
This switches us from JSHint to ESLint, improving the quality and
consistency of our code. (We also use ESLint elsewhere at Airtable.)

To break down this change:

* We made several cleanups before this commit to ease the transition.
  See [#95][1], [#96][2], [#97][3], [#98][4], [#99][5], [#101][6] (and a
  fix in [#107][7]), [#105][8], [#109][9], [#118][10], [#119][11],
  [#120][12]
* Installed `eslint`
* Add `.eslintrc.json` and `.eslintignore` (and a slightly-different
  `.eslintrc.json` in `test`)
* Remove JSHint. Involved uninstalling the package, removing it from the
  gruntfile, and removing `jshint ignore`s.
* Update the `lint` task in `package.json`
* Update `return` into `return void 0` for consistent returns in
  `callbackToPromise`
* Add one `// eslint-disable-line` comment in a single test file
* Run `eslint --fix .`, automatically fixing all of the remaining
  warnings and errors

[1]: #95
[2]: #96
[3]: #97
[4]: #98
[5]: #99
[6]: #101
[7]: #107
[8]: #105
[9]: #109
[10]: #118
[11]: #119
[12]: #120
@EvanHahn EvanHahn mentioned this pull request Jul 9, 2019
Merged
EvanHahn pushed a commit that referenced this pull request Jul 10, 2019
Switch from JSHint to ESLint (#122)
52eb624 
This switches us from JSHint to ESLint, improving the quality and
consistency of our code. (We also use ESLint elsewhere at Airtable.)

To break down this change:

* We made several cleanups before this commit to ease the transition.
  See [#95][1], [#96][2], [#97][3], [#98][4], [#99][5], [#101][6] (and a
  fix in [#107][7]), [#105][8], [#109][9], [#118][10], [#119][11],
  [#120][12]
* Install `eslint`
* Add `.eslintrc.json` and `.eslintignore` (and a slightly-different
  `.eslintrc.json` in `test`)
* Remove JSHint. Involved uninstalling the package, removing it from the
  gruntfile, and removing `jshint ignore`s.
* Update the `lint` task in `package.json`
* Update `return` into `return void 0` for consistent returns in
  `callbackToPromise`
* Add one `// eslint-disable-line` comment in a single test file
* Run `eslint --fix .`, automatically fixing all of the remaining
  warnings and errors

[1]: #95
[2]: #96
[3]: #97
[4]: #98
[5]: #99
[6]: #101
[7]: #107
[8]: #105
[9]: #109
[10]: #118
[11]: #119
[12]: #120
PeterPan627 added a commit to PeterPan627/airtable.js that referenced this pull request May 28, 2023
@PeterPan627
Switch from JSHint to ESLint (#122)
5a7a0a0 
This switches us from JSHint to ESLint, improving the quality and
consistency of our code. (We also use ESLint elsewhere at Airtable.)

To break down this change:

* We made several cleanups before this commit to ease the transition.
  See [#95][1], [#96][2], [#97][3], [#98][4], [#99][5], [#101][6] (and a
  fix in [#107][7]), [#105][8], [#109][9], [#118][10], [#119][11],
  [#120][12]
* Install `eslint`
* Add `.eslintrc.json` and `.eslintignore` (and a slightly-different
  `.eslintrc.json` in `test`)
* Remove JSHint. Involved uninstalling the package, removing it from the
  gruntfile, and removing `jshint ignore`s.
* Update the `lint` task in `package.json`
* Update `return` into `return void 0` for consistent returns in
  `callbackToPromise`
* Add one `// eslint-disable-line` comment in a single test file
* Run `eslint --fix .`, automatically fixing all of the remaining
  warnings and errors

[1]: Airtable/airtable.js#95
[2]: Airtable/airtable.js#96
[3]: Airtable/airtable.js#97
[4]: Airtable/airtable.js#98
[5]: Airtable/airtable.js#99
[6]: Airtable/airtable.js#101
[7]: Airtable/airtable.js#107
[8]: Airtable/airtable.js#105
[9]: Airtable/airtable.js#109
[10]: Airtable/airtable.js#118
[11]: Airtable/airtable.js#119
[12]: Airtable/airtable.js#120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kasrak kasrak kasrak approved these changes

@jbbakst jbbakst Awaiting requested review from jbbakst

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EvanHahn @kasrak