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

Upgrade to react-static 6 #859

Closed

Conversation

NavyAdmiral
Copy link
Contributor

There are a lot of breaking changes in 6 so it's not an easy upgrade. I made the dev-server work, but build is still failing. That is the only thing left to fix in this PR.

@rupurt Resize package is not working properly anymore so you might want to take a look. I commented it out as it was making the website crash.

Also @rupurt react-static no longer ships with routing capabilities so we need to start using our own. I added react-router-dom, but couldn't make it fully work yet as it's still failing production build on exporting html. Should we instead use the @reach/router as react-static team recommends?

gui/src/Layout.js Outdated Show resolved Hide resolved
gui/src/Layout.js Outdated Show resolved Hide resolved
@rupurt
Copy link
Contributor

rupurt commented Jan 3, 2019

@NavyAdmiral awesome cheers 🍺

👍 to @reach/router. It's from the same guy who built a lot of react-router and the API looks fairly similar with a few nice enhancements.

I'll take a deeper look at the PR today.

Also thanks for the input @tannerlinsley 😄

@rupurt
Copy link
Contributor

rupurt commented Jan 4, 2019

@NavyAdmiral I think a lot of the failures are due to importing Router from react-static in the tests e.g.

// gui/__tests__/containers/Jobs/Index.test.js
import { Router } from 'react-static'

@NavyAdmiral NavyAdmiral force-pushed the features/react-static-6 branch 3 times, most recently from 8c3129e to 20639f0 Compare January 5, 2019 05:20
@NavyAdmiral NavyAdmiral changed the title [WIP] Upgrade to react-static 6 Upgrade to react-static 6 Jan 5, 2019
@NavyAdmiral
Copy link
Contributor Author

@rupurt Do you have any idea why just switching to @babel/register from babel-register and updating the imports correspondingly isn't enough?

@rupurt
Copy link
Contributor

rupurt commented Jan 9, 2019

@NavyAdmiral the geth tests run the e2e tests. You can replicate this by running yarn test-e2e locally. It's failing because the link in the success flash banner is hidden behind the <Header> and can't be clicked by the user.

The incorrect positioning is due to the skipOnMount prop getting set. I took a bit of a look into why that causes the problem but couldn't find a smoking gun at this point and need to time box this. There is an open discussion around something that looks similar to the issue we're running into here.

Also, standard linting errors are unrelated to this PR. They are all over other PRs as well

Check gui tests here https://circleci.com/gh/smartcontractkit/chainlink/14084?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

It looks like the lint task has always been returning a 0 exit code due to snazzy. I've created a new WIP PR to fix it #883

Finally, are there any plans to restructure chainlink? It's already quite heavy. Storybook, truffle, gui, geth, flatteners, solidity, et al, all in 1 package results in unrelated upgrades breaking each other like it did now.

There are currently no plans to restructure the project but we've discussed it. We previously had multiple package.json files but merged them together to get uniform tooling across the project.

@NavyAdmiral
Copy link
Contributor Author

NavyAdmiral commented Jan 10, 2019

@rupurt The thing is it used to work fine without skipOnMount and for some reason it broke with this PR. A wild shot at this would be there are some React 16.7 specific changes that broke it. Wilder shot would be it's somehow caused by functional components instead of classes? I'm not quite sure about this because as far as I know both should behave the same unless the class was a <PureComponent/> (which it wasn't before we switched to hooks)

Geth failing is okay as that can be fixed, but I don't know why truffle's failing

@NavyAdmiral NavyAdmiral force-pushed the features/react-static-6 branch 2 times, most recently from da4b456 to 28cd289 Compare January 10, 2019 14:54
@NavyAdmiral
Copy link
Contributor Author

Package react-use-hooks no longer needed with RS6. Waiting on react-static/react-static#958

@rupurt
Copy link
Contributor

rupurt commented Jan 24, 2019

Closing in favor of #919

@rupurt rupurt closed this Jan 24, 2019
asoliman92 pushed a commit that referenced this pull request Jul 31, 2024
## Motivation

The `GetTransfers()` functions (bridges), are lacking checks that the L1
and L2 liquidity manager tokens match the addresses provided as
`remoteToken` and `localToken`.

## Solution

Get liquidity managers token in bridge constructor and save for reuse,
upon calls to `GetTransfer()` use the address to compare with
`remoteToken` and `localToken`.
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