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

Support React Hooks (#5602) #5997

Merged
merged 15 commits into from
Mar 15, 2019
Merged

Conversation

EivindArvesen
Copy link
Contributor

@EivindArvesen EivindArvesen commented Dec 9, 2018

Ref. #5602

  • Add ESLint plug-in
  • Add Babel plug-in that would force array destructuring to work in loose mode for known Hooks

ESLint plugin should probably be behind an option in the preset, but enabled in CRA by default – this is not yet implemented.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@iansu iansu added this to the 3.0 milestone Dec 10, 2018
@Pajn
Copy link
Contributor

Pajn commented Jan 10, 2019

Doesn't this enable loose mode for all destructuring operations?

This can be very breaking and diverges from ES6 quite a bit. Instead it should only be enabled when destructuring from useState and equivalent.

@EivindArvesen
Copy link
Contributor Author

You're right. Do you know how I might go about achieving it only for known hooks?
I thought there might be some option of specifying the known hooks in the plugin config somehow, but that doesn't appear to be the case.

@EivindArvesen
Copy link
Contributor Author

Ping @Pajn

@Pajn
Copy link
Contributor

Pajn commented Feb 7, 2019

Sorry, I completely missed the first notification.

I'm not too knowledgeable with the babel plugins or AST but I imagine that you would have to create a custom babel plugin (based on the @babel/plugin-transform-destructuring code). Some pseudo logic would be: check that the RHS of the assignment statement which does the destructuring is a function call which is a whitelisted hook name (like useState) and if so transpile that statement in loose mode, otherwise use the full transformation.

@lvl99
Copy link

lvl99 commented Feb 10, 2019

@eivind88 based on @Pajn's advice, I've adapted babel-plugin-transform-destructuring to take a whitelist of property/function names to apply the loose rule when destructuring.

Have a look at lvl99/babel@e2600c3 and let me know if it could be used here.

Edit: PR on babel available here: babel/babel#9486

@EivindArvesen
Copy link
Contributor Author

@lvl99 Great!

I've updated my PR; Could you have a look and verify that I've understood your change correctly? :)

This PRs dependency versions will need to be bumped after you PR is merged.

@lvl99
Copy link

lvl99 commented Feb 17, 2019

@eivind88 I think since you have loose mode enabled on this line that it would always be enabled, even with the selectiveLoose values given, due to this line I have here.

Perhaps I need to rethink what exactly is happening or how we want to use it. isLoose would only be true if the selectiveLoose option is set and the checked item matches the selectiveLoose list, otherwise if it is false it will defer to the loose value (in your PR you have loose: true so it would end up running all destructuring in loose mode).

Maybe my isLoose conditional should be a bit more selective, by returning a false if the item is not on the selectiveLoose list even if the loose option is true:

const isLoose = this.selectiveLoose
  ? checkNameMatchesSelectiveLoose(objRef.name, this.selectiveLoose))
  : loose;

I think that would make more sense!

Edit: otherwise, if you set loose: false and then keep the selectiveLoose option as it, I think it should then run as we expect by only destructuring the functions that match those given on the selectiveLoose list loosely.

@EivindArvesen
Copy link
Contributor Author

@lvl99 Updated.

Where do we go from here, considering that changes will need to be merged into Babel before this is usable?

@lvl99
Copy link

lvl99 commented Feb 21, 2019

@eivind88 not sure! Maybe we could post comments on the PR and try and get others like @gaearon to help get it assessed for appropriateness and (hopefully) merged into Babel.

@EivindArvesen
Copy link
Contributor Author

EivindArvesen commented Feb 28, 2019

@iansu Any thoughts?😃

@iansu
Copy link
Contributor

iansu commented Feb 28, 2019

We are also going to want to add this rule: "react-hooks/exhaustive-deps": "warn"

@iansu iansu self-assigned this Feb 28, 2019
@EivindArvesen
Copy link
Contributor Author

@iansu: Like so?

@iansu
Copy link
Contributor

iansu commented Mar 7, 2019

A stable version of eslint-plugin-react-hooks has just been released. Can you update your PR to use version 1.5.0? Once that’s done I think we’re ready to merge.

@EivindArvesen
Copy link
Contributor Author

Done!

@iansu
Copy link
Contributor

iansu commented Mar 7, 2019

Thanks! I'm just looking into the unit test failures. They're not related to your changes but I'd like to fix them before we merge.

@ianschmitz
Copy link
Contributor

I think there's overlap with the babel plugin here and the work being done in #6219? If so we probably want to remove that change from this PR and focus on the ESLint plugin side of things.

@iansu
Copy link
Contributor

iansu commented Mar 11, 2019

@ianschmitz Are you referring to the loose mode destructuring part? I don't think #6219 is going to make it into 3.0 so I'm thinking we include this for now and then remove it in #6219. What do you think?

@ianschmitz
Copy link
Contributor

@iansu yes i'm referring to the babel plugin that was added here. If we don't think #6219 will make it in for 3.0 then it probably makes sense to include it and later remove like you said.

@iansu iansu closed this Mar 11, 2019
@iansu iansu reopened this Mar 11, 2019
Copy link
Contributor

@petetnt petetnt left a comment

Choose a reason for hiding this comment

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

Small comment, otherwise codewise LGTM

packages/react-scripts/package.json Outdated Show resolved Hide resolved
@iansu iansu merged commit 7ae3146 into facebook:master Mar 15, 2019
@iansu
Copy link
Contributor

iansu commented Mar 15, 2019

Thanks!

@iansu iansu mentioned this pull request Mar 15, 2019
JoviDeCroock added a commit to JoviDeCroock/create-react-app that referenced this pull request Mar 15, 2019
* masterd: (24 commits)
  Add TypeScript linting support (facebook#6513)
  Support React Hooks (facebook#5602) (facebook#5997)
  Support browserslist in @babel/preset-env (facebook#6608)
  Add empty mock for http2 (facebook#5686)
  Add note about npx caching (facebook#6374)
  change named import into default import (facebook#6625)
  Stage files for commit after ejecting (facebook#5960)
  Upgrade dependencies (facebook#6614)
  Make compiler variable const instead of let (facebook#6621)
  Type check JSON files (facebook#6615)
  Change class components to functional components in templates (facebook#6451)
  Convert JSON.stringify \n to os.EOL when writing tsconfig.json (facebook#6610)
  Update html-webpack-plugin (facebook#6361)
  Enable click to go to error in console for TypeScript (facebook#6502)
  Update webpack-dev-server to 3.2.1 (facebook#6483)
  [docs] revert removal of newlines from html (facebook#6386)
  Publish
  Prepare 2.1.8 release
  Reapply "Speed up TypeScript v2 (facebook#6406)" (facebook#6586)
  Publish
  ...

# Conflicts:
#	packages/babel-preset-react-app/create.js
#	packages/react-scripts/scripts/build.js
@lock lock bot locked and limited conversation to collaborators Mar 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants