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

feat: Adding codemods in loadable-codemod #463

Merged
merged 8 commits into from
Jan 9, 2020

Conversation

jackyef
Copy link
Contributor

@jackyef jackyef commented Nov 16, 2019

Summary

This package is a collection of codemod using jscodeshift that can be used to help making big changes easier to a project, for example: migrating from react-loadable to @loadable/component

This PR setups this in a new package named loadable-codemod and provide the react-loadable-to-loadable-component transform.

Test plan

All tests are put inside codemod/transform/__tests__.

Tests are written as such:

jest.autoMockOff();

const { defineTest } = require('jscodeshift/dist/testUtils');

defineTest(__dirname, 'react-loadable-to-loadable-component', null, 'react-loadable-to-loadable-component_expr');

This will check for the file react-loadable-to-loadable-component_expr.input.js inside @loadable/codemod/transform/__testfixtures__, feed it into the react-loadable-to-loadable-component transformer, and assert that the output is equal to react-loadable-to-loadable-component_expr.output.js

Notes

There might be some cases that I haven't accounted for, so any help in improvement would be great! cc: @neoziro

As per @jonatansberg's input, instead of rendering null when there is a use case where @loadable/component can't handle, we will render a reasonable default, while logging warning messages.

image

The props we will be passing to the loading component will be as following:

{
  pastDelay: true,
  error: false,
  timedOut: false,
}


const CustomLinkLoadable = loadable(() =>
import(/* webpackChunkName: "custom-link" */ '@components/CustomLink/Link'), {
fallback: null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should throw in this case, as long as some user logic is going to be lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I am having quite a bit of dilemma with that one. I assume if we were to throw in that case, the codemod would be quite unusable.

I do not have a number, but I assume the number of people using that pattern should be quite big, especially because it's in the README.md of the project. 🤔

Would be happy to get additional perspective regarding this one!

Copy link

@jonatansberg jonatansberg Dec 3, 2019

Choose a reason for hiding this comment

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

Would it make sense to solve this by extending loadable-components so that the fallback can be either an element (like today) or a render function/component, like react-loadable?

Something like React.isValidElement(fallback) ? fallback : React.createElement(fallback, props) should be enough. I guess the problem might be figuring out whatever props should be...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since react-loadable has some behaviours that don't exist in @loadable/component, I am not sure if there is a way to make the change without changing some logics; unless of course @loadable/component adds the same APIs that implement the same behaviours.

Choose a reason for hiding this comment

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

Yeah, I get that. A sort of halfway there solution could be to wrap the inline function/loading component in a React.createElement call with no props. This would at least render the default state and preserve the existing code.

The transform should probably still write something to the console about that loading components aren't fully supported and that additional action might be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added it! Please kindly review it again 🙂

@theKashey
Copy link
Collaborator

I like the idea 🤩

@jackyef jackyef changed the title feat: Adding codemods feat: Adding codemods in @loadable/codemod Nov 16, 2019
@jackyef jackyef changed the title feat: Adding codemods in @loadable/codemod feat: Adding codemods in loadable-codemod Nov 16, 2019
@theKashey
Copy link
Collaborator

I think there is only one step before getting the world's best, as well as the world's first codemod of this type - react-loadable-to-loadable-component migration guide in the docs (which does not exist right now).

  • the first line of that page would be: "Use codemod"
  • the second line would be about limitations like we are not able, and not going to convert all cases
  • the third one would be about which patterns from the previous point could be converted to some loadable variant, and which ones should be just refactored.

@jackyef
Copy link
Contributor Author

jackyef commented Dec 13, 2019

Sorry for the late reply, work has been busy these past few weeks 😞

Alright, from what I read, the summary is:

  1. Instead of rendering null make it so we render the component without passing any props.
  2. Logs out warning when there are changes that isn't 100% equivalent, such as the one in point 1.
  3. Add documentation about Use codemod

I will try to find the time to work on this, hopefully soon.

@gregberge gregberge merged commit a82d5ad into gregberge:master Jan 9, 2020
@gregberge
Copy link
Owner

Thanks!

@jackyef jackyef deleted the codemod branch January 10, 2020 05:01
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