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

Removed most instances of 'self' #615

Closed
wants to merge 9 commits into from
Closed

Conversation

paulasjes-stripe
Copy link
Contributor

Removed almost all cases of const self = this; since we're (mostly) using arrow functions now.

Notable exceptions are functions that are getting removed in #613 (e.g. setMetadata) and functions which require the scope to be untouched (most of the Mocha test suite functions).

Ran the tests in Node 6 and Node 8, leaning quite heavily on them here.

r? @rattrayalex-stripe @ob-stripe

rattrayalex-stripe and others added 7 commits May 8, 2019 00:03
* 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
Copy link
Contributor

Sorry, hope to look at this ~tomorrow...

@rattrayalex-stripe rattrayalex-stripe mentioned this pull request May 9, 2019
13 tasks
@rattrayalex-stripe
Copy link
Contributor

LGTM

though if we can get that last one as an arrow function that'd be nice too

@paulasjes-stripe
Copy link
Contributor Author

@rattrayalex-stripe I was getting some weird errors with Nock when makeRequest was an arrow function rather than a regular function. I dug into them and found that it was simply a case of temporal dead zone where makeRequest was being called before it was defined 🤦‍♂.

Should be ready for re-review now.

@rattrayalex-stripe
Copy link
Contributor

rattrayalex-stripe commented May 14, 2019

@paulasjes-stripe Still seeing a ReferenceError: self is not defined in Travis.

What do you think about holding off on this until after 7.0.0 lands? Eg; shipping as part of 7.0.1. Feels like it may be a bit riskier, might be good to isolate it a bit from the other changes (some of which are also risky; would like to make it easier to debug should an issue arise).

@rattrayalex-stripe rattrayalex-stripe force-pushed the v7.0.0 branch 2 times, most recently from 762b6a9 to f780ff3 Compare May 14, 2019 18:40
@rattrayalex-stripe
Copy link
Contributor

Per DM, we're going to hold off on this until 7.0.0 has been released and live for a few days.

@rattrayalex-stripe rattrayalex-stripe force-pushed the v7.0.0 branch 2 times, most recently from 9fdad65 to cdea81a Compare May 14, 2019 23:08
@paulasjes-stripe
Copy link
Contributor Author

Closing this as will be merging into master rather than the old 7.0.0 branch.

@ob-stripe ob-stripe deleted the remove-self 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