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

chore: upgrade dependencies #583

Merged
merged 3 commits into from
Jun 15, 2020
Merged

chore: upgrade dependencies #583

merged 3 commits into from
Jun 15, 2020

Conversation

CreativeTechGuy
Copy link
Contributor

For the past several weeks CI has been failing at the installation stage with the error:

[!] (plugin babel) Error: Cannot find module '@babel/compat-data/corejs3-shipped-proposals'

In an attempt to fix this, I opted to update the dependencies to see if that solves the issue. All dependencies are updated to the latest versions. This mostly affects the tests which needed to be updated as it now used deprecated features. I diffed the output bundles before and after the change and there was no meaningful difference.

Note, there is one failing useQuery test (should reset error state if new component instances are mounted) which I presume is failing due to a timing issue with this setTimeout. I was not able to fix this test but at least now we should be able to see accurate CI results that it's failing and can follow up with fixing the test or the library. This test error was introduced with this upgrade, but only in the unit tests as the output bundle has no change.

@CreativeTechGuy
Copy link
Contributor Author

Interesting, the preactjs/compressed-size-action still fails with the same error during the build step but the node 12 ubuntu-latest test successfully installs and fails at the useQuery test as expected (see above).

Still needs work, but I think it's closer. @tannerlinsley where do we go from here?

@CreativeTechGuy CreativeTechGuy marked this pull request as ready for review June 14, 2020 20:40
@tannerlinsley
Copy link
Collaborator

Let’s delete the size workflow. Useless tbh

@CreativeTechGuy
Copy link
Contributor Author

I'm glad you said that because I didn't understand it either haha. But do you have any idea how to fix the suspense hook? I wasn't able to reproduce the problem in a codesandbox so my best guess is that it's a issue with one of the test libraries and possibly re-rendering twice before the setTimeout is able to fire.

The best I was able to do was:

fireEvent.click(rendered.getByText('retry'))
await sleep(100)
fireEvent.click(rendered.getByText('retry'))

But this seems like a really bad solution as it's no longer a representative test of reality.

@tannerlinsley
Copy link
Collaborator

tannerlinsley commented Jun 14, 2020 via email

@CreativeTechGuy
Copy link
Contributor Author

I don't think we should leave tests failing that we expect to fail. That's a really bad impression to give off.

I'd vote to mark the suspense features in the docs as experimental (because suspense for data fetching itself is experimental) and disable (or rewrite) the tests for now. If someone wants to use those features it's buyer beware. The concern with this approach is that there may be people using the feature in production and losing confidence by removing tests isn't good.

@tannerlinsley
Copy link
Collaborator

tannerlinsley commented Jun 14, 2020 via email

@CreativeTechGuy
Copy link
Contributor Author

Removed workflow, added workaround for test with explanation.

The CI tests now succeed!

@tannerlinsley
Copy link
Collaborator

This is great! Would you mind applying this to the next branch too?

@tannerlinsley tannerlinsley merged commit 06feac8 into TanStack:master Jun 15, 2020
@CreativeTechGuy
Copy link
Contributor Author

What is the next branch? I couldn't find the process for that documented of what goes there.

@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 1.5.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tannerlinsley
Copy link
Collaborator

tannerlinsley commented Jun 15, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants