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

Relay 9 #155

Merged
merged 1 commit into from Feb 27, 2020
Merged

Relay 9 #155

merged 1 commit into from Feb 27, 2020

Conversation

RDIL
Copy link
Contributor

@RDIL RDIL commented Feb 21, 2020

Signed-off-by: Reece Dunham [email protected]

New in this PR 🔥

Feature 🖥

  • Relay v9
  • Synced graphql schema
  • TypeScript 3.8

Dependencies ⛽️

  • Update @material-ui/core
  • Update Jest types
  • Update NodeJS types
  • Update React types
  • Update the stripe API package

Bug Fixes 🐞

  • Docker: source maps are generated by default

@RDIL RDIL requested a review from fkorotkov February 21, 2020 21:08
@fkorotkov
Copy link
Contributor

I'm hesitant about switching to react-loadable. Yeutech Lab
just copied it without even mentioning it's a fork and keeping a list of alleged users from the original library.

This project was originally hosted at https://github.com/jamiebuilds/react-loadable

^^ sounds like they now officially maintain the project but I didn't find any confirmation. Based on this comment it seems they just decided to copy the project without even properly forking and merge in the change. There were no changes since the fork was create and don't have any certainty "this community fork" will be better maintained.

@RDIL
Copy link
Contributor Author

RDIL commented Feb 26, 2020

Actually, they do credit the original project, see https://github.com/yeutech-lab/react-loadable#why-did-we-fork

@RDIL
Copy link
Contributor Author

RDIL commented Feb 26, 2020

Its a community fork, they fixed the bugs and left it, I don't see why not updating the list of users is so bad. They made the changes needed, and left it.

@fkorotkov
Copy link
Contributor

Here are my concerns about using this "fork":

  • There is 86 weekly downloads of the package from NPM
  • It was copied by an individual with only one change that he wanted to merge.
  • IMO it was weirdly forked. Copied with these two lines about the original project while keeping the list of users.
  • All of the above makes me hard to call this project coming from react-loadable community yet.
  • React 17 is not yet release and this upgrade is not at all related to the matter of this issue: update Relay.

Signed-off-by: Reece Dunham <[email protected]>
@RDIL
Copy link
Contributor Author

RDIL commented Feb 27, 2020

Fixed

@fkorotkov
Copy link
Contributor

Regarding the react-loadable. It seems https://github.com/gregberge/loadable-components worth investigation.

@fkorotkov fkorotkov merged commit 9b4c616 into cirruslabs:master Feb 27, 2020
@RDIL RDIL deleted the typical branch February 27, 2020 17:55
@velusgautam
Copy link

https://www.npmjs.com/package/react-loadable-hooks implementation with React 17

@fkorotkov
Copy link
Contributor

@velusgautam ended up switching to React's lazy right after is #159

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.

3 participants