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

Modernize ES5 to ES6 with lebab #607

Merged
merged 18 commits into from
May 8, 2019
Merged

Modernize ES5 to ES6 with lebab #607

merged 18 commits into from
May 8, 2019

Conversation

rattrayalex-stripe
Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe commented May 4, 2019

Builds on #604, targeting the v7.0.0 release

Uses https://github.com/lebab/lebab, running each transform in its own commit.

See the script I added in the first commit (and removed in the last) for the process.

The primary benefits are, IMO, var->const, function->=> and {foo: foo}->{foo}.

cc @stripe/api-libraries
cc @paulasjes-stripe @robz-stripe @ob-stripe

I'm putting a lot of faith in the test suite here, and my confidence isn't 100% that this is bug-free. I'm not totally sure how to proceed with confidence, or who should review.

EDIT: looking through these commit-by-commit, a few I'm reverting because I think they're bad/dangerous.

These I'm not sure about:

  1. c1934de I think I should just carefully check that there's code coverage for any possible bugs introduced here. I like the change and would like to keep it.
  2. 08e4905 Relatedly, I love this change but do think there's some possibility it could introduce some subtle edge-case bugs. This one however is way too big to audit. I think any bugs that the test suite wouldn't have caught would be of the shape of "a user customizes their stripe-node setup in some way that we support but don't test and there was some implicit reliance on this". I'm not sure how large the surface area for this is...

Note that I made a few changes by hand (which I probably should have done in separate commits in retrospect). They were:

  1. This needed to stay function instead of arrow functions:
    module.exports = function(namespace, resources) {
    return function(stripe) {
    return new ResourceNamespace(stripe, resources);
    };
    };
  2. These needed to move to const (lebab moved var to let, then eslint complained that they were not reassigned):
    const apiVersion = self._stripe.getApiField('version');
    requestData = data;
    const headers = self._defaultHeaders(
  3. This needed to stay a function:
    constructor: function() {

@rattrayalex-stripe rattrayalex-stripe changed the title Modernize ES5 to ES5 with lebab Modernize ES5 to ES6 with lebab May 4, 2019
@rattrayalex-stripe
Copy link
Contributor Author

cc @irace-stripe FYI – looks like it should mostly affect you in that it translates var to const at the top of the files.

@paulasjes-stripe
Copy link
Contributor

I ran the tests on node v6.0.0 and v8.0.0. Works as expected in v6, broke on autopagination tests in v8. Turns out it's a node error, upgrading to v8.1 fixed the issue.

Perhaps we should exclude v8 in favour of v8.1.0?

@irace-stripe
Copy link

cc @irace-stripe FYI – looks like it should mostly affect you in that it translates var to const at the top of the files.

Noted – a change I’m happy to make!

@@ -50,7 +50,7 @@ StripeResource.prototype = {
// Methods that don't use the API's default '/v1' path can override it with this setting.
basePath: null,

initialize: function() {},
initialize() {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd either revert this or change it to initialize: () => {},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I prefer that style too... Doesn't seem lebab supports it, I think. Will try to see if I can find an eslint autofix...

type: 'StreamProcessingError',
populate: function(raw) {
populate(raw) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit like before, but I prefer populate: (raw) => {

@rattrayalex-stripe rattrayalex-stripe changed the base branch from irace-prettier-formatting to v7.0.0 May 8, 2019 00:51
@rattrayalex-stripe
Copy link
Contributor Author

@paulasjes-stripe in 6f2dcb6 I used arrow syntax for more methods; there weren't many places I could safely do this.

I also tried to migrate from self to this, but tests started failing and I aborted before rabbit-holing too deeply. Would appreciate a follow-on PR (no urgency, can happen post-release) finishing this if you're willing to submit one!

r? @ob-stripe

@rattrayalex-stripe
Copy link
Contributor Author

Per IRL,

LGTM

@rattrayalex-stripe rattrayalex-stripe merged commit cfd1eb5 into v7.0.0 May 8, 2019
ob-stripe pushed a commit that referenced this pull request May 8, 2019
* Add lebab and a script to run it

* lebab transform: arrow

* lebab transform: arg-rest

* lebab transform: arg-spread

* lebab transform: obj-method

* lebab transform: obj-shorthand

* lebab transform: let

* lebab transform: template

* lebab transform: default-param

* lebab transform: destruct-param

* lebab transform: includes

* Revert "Add lebab and a script to run it"

This reverts commit 70fd492.

* Revert "lebab transform: destruct-param" because its changes didn't seem good.

This  reverts commit b56f52d.

* Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible.

This reverts commit 7eba992.

* Unrelated: mark 8.1 as minimum 8-series version

* Add mocha-only script

* Use arrows in more places

* Loosen some eslint rules I don't love
ob-stripe pushed a commit that referenced this pull request May 8, 2019
* Add lebab and a script to run it

* lebab transform: arrow

* lebab transform: arg-rest

* lebab transform: arg-spread

* lebab transform: obj-method

* lebab transform: obj-shorthand

* lebab transform: let

* lebab transform: template

* lebab transform: default-param

* lebab transform: destruct-param

* lebab transform: includes

* Revert "Add lebab and a script to run it"

This reverts commit 70fd492.

* Revert "lebab transform: destruct-param" because its changes didn't seem good.

This  reverts commit b56f52d.

* Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible.

This reverts commit 7eba992.

* Unrelated: mark 8.1 as minimum 8-series version

* Add mocha-only script

* Use arrows in more places

* Loosen some eslint rules I don't love
rattrayalex-stripe added a commit that referenced this pull request May 14, 2019
* Add lebab and a script to run it

* lebab transform: arrow

* lebab transform: arg-rest

* lebab transform: arg-spread

* lebab transform: obj-method

* lebab transform: obj-shorthand

* lebab transform: let

* lebab transform: template

* lebab transform: default-param

* lebab transform: destruct-param

* lebab transform: includes

* Revert "Add lebab and a script to run it"

This reverts commit 70fd492.

* Revert "lebab transform: destruct-param" because its changes didn't seem good.

This  reverts commit b56f52d.

* Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible.

This reverts commit 7eba992.

* Unrelated: mark 8.1 as minimum 8-series version

* Add mocha-only script

* Use arrows in more places

* Loosen some eslint rules I don't love
rattrayalex-stripe added a commit that referenced this pull request May 14, 2019
* Add lebab and a script to run it

* lebab transform: arrow

* lebab transform: arg-rest

* lebab transform: arg-spread

* lebab transform: obj-method

* lebab transform: obj-shorthand

* lebab transform: let

* lebab transform: template

* lebab transform: default-param

* lebab transform: destruct-param

* lebab transform: includes

* Revert "Add lebab and a script to run it"

This reverts commit 70fd492.

* Revert "lebab transform: destruct-param" because its changes didn't seem good.

This  reverts commit b56f52d.

* Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible.

This reverts commit 7eba992.

* Unrelated: mark 8.1 as minimum 8-series version

* Add mocha-only script

* Use arrows in more places

* Loosen some eslint rules I don't love
rattrayalex-stripe added a commit that referenced this pull request May 14, 2019
* Add lebab and a script to run it

* lebab transform: arrow

* lebab transform: arg-rest

* lebab transform: arg-spread

* lebab transform: obj-method

* lebab transform: obj-shorthand

* lebab transform: let

* lebab transform: template

* lebab transform: default-param

* lebab transform: destruct-param

* lebab transform: includes

* Revert "Add lebab and a script to run it"

This reverts commit 70fd492.

* Revert "lebab transform: destruct-param" because its changes didn't seem good.

This  reverts commit b56f52d.

* Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible.

This reverts commit 7eba992.

* Unrelated: mark 8.1 as minimum 8-series version

* Add mocha-only script

* Use arrows in more places

* Loosen some eslint rules I don't love
rattrayalex-stripe added a commit that referenced this pull request May 14, 2019
* Update version to 7 and drop support for Node 4 and Node 5, and Node 7

* Format using Prettier

tmp

* Alphabetize basic methods

* Use 'id' for all single identifier positional arguments

* Fix typo

* Modernize ES5 to ES6 with lebab (#607)

* Add lebab and a script to run it

* lebab transform: arrow

* lebab transform: arg-rest

* lebab transform: arg-spread

* lebab transform: obj-method

* lebab transform: obj-shorthand

* lebab transform: let

* lebab transform: template

* lebab transform: default-param

* lebab transform: destruct-param

* lebab transform: includes

* Revert "Add lebab and a script to run it"

This reverts commit 70fd492.

* Revert "lebab transform: destruct-param" because its changes didn't seem good.

This  reverts commit b56f52d.

* Revert "lebab transform: default-param" because it seems dangerous / backwards-incompatible.

This reverts commit 7eba992.

* Unrelated: mark 8.1 as minimum 8-series version

* Add mocha-only script

* Use arrows in more places

* Loosen some eslint rules I don't love

* Remove deprecated methods

* Add VSCode and EditorConfig files

* Bump dependencies to latest versions

* Remove legacy parameter support in invoices.retrieveUpcoming()

* Misc. manual formatting (#623)

* Misc. manual formatting

* Fix some unit tests

* Roll back path argument name changes

* Misc. manual formatting

* Remove "curried" nested resources and manually specified urlParams (#625)

* Drop support for optional url params

* Delete nested resource files

* Remove urlData

* Extract urlParams from path instead of manual definition

Verified this is no different with:

```js
const urlParams = utils.extractUrlParams(spec.path || '');
if (
  !(spec.urlParams || []).every((x, i) => urlParams[i] === x) ||
  (spec.urlParams || []).length !== urlParams.length
) {
  throw Error(
    'mismatch' + JSON.stringify(urlParams) + JSON.stringify(spec.urlParams)
  );
}
```

inside StripeMethod

* Remove manually specified urlParams

* Add a deprecation error message

* Revert "Delete nested resource files"

This reverts commit d88a3e7.

* Fix nested resources for non-curried urlParams and update tests to demonstrate their use

* Refactor makeRequest

* Revert "Revert "Delete nested resource files""

This reverts commit e5eccb8.

* Extract resources file (to aid with code generation) (#626)

* Extract separate resources file (to aid with code generation)

* Remove resources that were removed in #625

https://github.com/stripe/stripe-node/pull/625/files#diff-d3dd6c4fd6f915f29d42e4081dc817a8L85

* Update CHANGELOG for 7.0.0 release
@ob-stripe ob-stripe deleted the ralex/lebab branch October 29, 2019 15:10
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.

4 participants