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

Remove Babel 6 from internal dependencies #7037

Merged
merged 3 commits into from
Aug 26, 2019

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Aug 19, 2019

@rekmarks rekmarks added DO-NOT-MERGE Pull requests that should not be merged area-buildSystem related to our build system labels Aug 19, 2019
@rekmarks rekmarks force-pushed the babel-dependencies branch from 38f6eb6 to b9b034f Compare August 21, 2019 05:42
@rekmarks rekmarks force-pushed the babel-dependencies branch from b9b034f to 75b93e4 Compare August 21, 2019 05:47
@rekmarks rekmarks removed the DO-NOT-MERGE Pull requests that should not be merged label Aug 21, 2019
@rekmarks
Copy link
Member Author

rekmarks commented Aug 21, 2019

Currently failing e2e tests.

Chrome: throws on this line of web3-stream-provider: https://github.com/kumavis/web3-stream-provider/blob/master/index.js#L54

Firefox: Timeout Error with reference to TimeoutError: Waiting for element to be located By(css selector, .unlock-page__link--import)

I can reproduce the Firefox error but not the Chrome error locally. Branch was rebased onto develop today (8/20/2019).

@kumavis
Copy link
Member

kumavis commented Aug 21, 2019

@rekmarks this could be caused by non-unique request ids if they are not going through an id-remapper

multiple requests with matching ids start, response for one deletes the stored callback, then the second response comes in and the callback is missing

potential solutions:

  • incomplete: validate that if a request comes in for an id when we're already waiting on that id, through at request time.
  • better: ensure we're using an id-remapper
  • alternate: drop web3 stream provider for something like dnode that tracks request response for us

@Gudahtt
Copy link
Member

Gudahtt commented Aug 21, 2019

I'm seeing this error locally as well, on both Chrome and Firefox.

Maybe one of these dependency changes is causing this duplicate request id issue? It wasn't clear to me what might have triggered this change.

@rekmarks
Copy link
Member Author

@Gudahtt I'm now able to repro as well. The Timeout Error appears to result from the web3-stream-provider error. Currently testing the hypothesis that we need to put an idRemapMiddleware somewhere.

@rekmarks
Copy link
Member Author

I believe I've identified the RPC requests with the duplicate IDs: https://github.com/MetaMask/eth-block-tracker/blob/master/src/polling.js#L69

Clearly these requests are passed through the extension UI's StreamProvider here: https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/ui.js#L129-L142

The question is, how are we processing these requests? I can't find the RpcEngine for the background connection on the UI-side. Adding an idRemapMiddleware to the background's provider connection middleware stack (see here and here and here) does nothing as the error is occurring in the UI.

@rekmarks rekmarks added DO-NOT-MERGE Pull requests that should not be merged and removed DO-NOT-MERGE Pull requests that should not be merged labels Aug 21, 2019
@rekmarks
Copy link
Member Author

rekmarks commented Aug 21, 2019

Added id remapping to web3-provider-stream, solving the duplicate id error: https://github.com/rekmarks/web3-stream-provider

Now something else broke, and upon review, my suspicion is that transactions are being created with undefined IDs, given that this error is thrown: https://github.com/MetaMask/metamask-extension/blob/develop/ui/app/ducks/confirm-transaction/confirm-transaction.duck.js#L353

It could be that because our now JSON RPC 2.0-compliant json-rpc-engine is escalating RPC errors, functionality relying on past behavior may be breaking.

Edit: The erroring function was called in componentDidMount of confirmTransaction. At the time of mounting the transactionId could still be undefined in some cases. Solved it by wrapping the function call in a condition.

@rekmarks rekmarks marked this pull request as ready for review August 21, 2019 20:36
@rekmarks
Copy link
Member Author

Ready for review.

e2e tests are passing with the same errors as for #7029

Still needs QA @tmashuang

@whymarrh, I don't want to mess with your RC, but?

Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

A few comments inline. Also can we rebase this on the latest develop?

package.json Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
update internal deps to latest published versions
@rekmarks rekmarks force-pushed the babel-dependencies branch from 962b62e to 7837cba Compare August 21, 2019 23:27
@rekmarks rekmarks requested review from whymarrh and kumavis August 24, 2019 19:54
.gitignore Outdated Show resolved Hide resolved
@rekmarks rekmarks force-pushed the babel-dependencies branch from 16fa0b6 to c8d7262 Compare August 26, 2019 02:40
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks good!

@Gudahtt Gudahtt dismissed whymarrh’s stale review August 26, 2019 14:44

This has been addressed

@rekmarks rekmarks merged commit 4e29fb9 into MetaMask:develop Aug 26, 2019
@rekmarks rekmarks deleted the babel-dependencies branch August 28, 2019 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-buildSystem related to our build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants