wix-incubator / vscode-glean

The extension provides refactoring tools for your React codebase
MIT License
1.46k stars 56 forks source link

cannot convert stateful component to hooks one #75

Closed RamyEl-basyouni closed 5 years ago

RamyEl-basyouni commented 5 years ago

Hi, I tried to use your experiential feature to convert class code base to hooks. As you explained, i tried to edit the settings file in VSCode located in Users/User_name/Library/Application Support/Code/User/settings.json with this JSON {

    "editor.suggestSelection": "first",
    "vsintellicode.modify.editor.suggestSelection": "automaticallyOverrodeDefaultValue",
    "terminal.integrated.rendererType": "dom",
    "javascript.updateImportsOnFileMove.enabled": "always",
    "glean.experiments": {
        "type": "hooksForFunctionalComponents"
    }
}

but always cannot find the option to convert stateful component to hook.

borislit commented 5 years ago

Hey @RamyEl-basyouni . Your configuration seems to be incorrect. As you can see in the docs, glean.experiments is an array of strings. https://github.com/wix/vscode-glean#gleanexperiments-default-

Hence, it should look as follows: "glean.experiments": ["hooksForFunctionalComponents"]

Tell me if this helps

jobokai commented 5 years ago

image I'm having a similar issue and notice this error message when editing the settings.json file. Doing a few conversions it then seems to have a pathing bug: image

Removing the setting then makes everything work again.

borislit commented 5 years ago

@jakutis, IMO You can ignore the formatting error. Can you please post the code snippet You are trying to refactor? Also, which OS are u using? Windows?

jobokai commented 5 years ago
import React from 'react';
import moment from 'moment';
class Copyright extends Component {
  constructor(props) {
    super(props);
  }
  render() {
    return (<div className='copyright'>
        <span className="purdue-logo"></span>
        <div className="copyright-statement">
          © {moment().format("YYYY")} .<br />
          text here.
        </div>
      </div>);
  }
}
export default Copyright;

I'm on a windows machine. Just wanting to test things out, it looks really interesting!

borislit commented 5 years ago

@jobokai This indeed looks like a bug, caused by the last release. Working on the fix now :) Ill keep you posted

borislit commented 5 years ago

:tada: This issue has been resolved in version 4.15.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

borislit commented 5 years ago

@jobokai @RamyEl-basyouni I've just published a new version that should include a fix. Please validate

jobokai commented 5 years ago

I’ll take a look here tonight when I get home from work! Thanks for checking on things

RamyEl-basyouni commented 5 years ago

Hi @borislit ,

First, i have upgraded to the latest version of glean@4.15.1 I tried to update my settings.json file as advised

Screen Shot 2019-08-06 at 10 08 03 AM

But i always cannot find the option of convert stateful to stateless component

Here is the screenshot of the options that is available on my codebase

Screen Shot 2019-08-06 at 10 21 09 AM

FYI, here is my code that i am trying to convert it to hook. https://gist.github.com/RamyEl-basyouni/fb6fe324c71e5b2cab15fb212f127201

borislit commented 5 years ago

@RamyEl-basyouni I've just checked the gist you've posted and it worked for me. Are you selecting JUST the component or the contents of the whole file?

borislit commented 5 years ago

@RamyEl-basyouni Also, in order to get a more better result, before applying the refactoring, I suggest you change all the instance variables i.e this.foo to state variables i.e this.state.foo. Glean currently doesnt treat class properties correctly. I plan to address it in the next version. Arguably, it makes more sense for some of the variables in Your gist anyways :)

RamyEl-basyouni commented 5 years ago

@borislit , ok it is working now, regarding your mention to

change all the instance variables i.e this.foo to state variables i.e this.state.foo. i think it will not be perfect to convert instance variables to state variables as the instance variables prevent component from rerendering component which will badly effects performance.

2012mjm commented 5 years ago

Hi @borislit ,

First, i have upgraded to the latest version of glean@4.15.1 I tried to update my settings.json file as advised

Screen Shot 2019-08-06 at 10 08 03 AM

But i always cannot find the option of convert stateful to stateless component

Here is the screenshot of the options that is available on my codebase

Screen Shot 2019-08-06 at 10 21 09 AM

FYI, here is my code that i am trying to convert it to hook. https://gist.github.com/RamyEl-basyouni/fb6fe324c71e5b2cab15fb212f127201

Fix your problem: https://github.com/wix/vscode-glean/pull/78

borislit commented 5 years ago

Merged @2012mjm ! Thanks for the PR!

M-Izadmehr commented 4 years ago

@borislit The settings schema expects glean.experiments to be string instead of array. I created a small MR #82 which solves this issue.