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

[jest-resolve] Add async resolver support #11540

Merged
merged 50 commits into from Feb 22, 2022

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented Jun 8, 2021

Summary

This is a stab at #9505. I would love to use jest in vite, and it sounds like this is the next step towards getting there.

I added async methods in the Resolver class and named them ending with Async, but I'm not sure if that's the kind of convention you want. It just helped me to keep everything straight, but I'm happy to rename them. Also, I wasn't super-consistent about putting async at the front or back of names, except that nouns I tried to put it at the front and verbs (methods) at the end. Because an option named resolverAsync sounded worse than asyncResolver. ¯\(ツ)/¯. Again, happy to adjust. Oh, and I didn't rename any of the existing methods, to keep backwards compat.

Fixes #9505

Test plan

I have copied the tests from the sync resolver, and adjusted them to work for the new async version. I didn't add any new test cases, though, but I'm happy to do so if there are holes.

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2021

Codecov Report

Merging #11540 (e58d96b) into main (d7f0975) will decrease coverage by 0.13%.
The diff coverage is 51.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11540      +/-   ##
==========================================
- Coverage   68.93%   68.79%   -0.14%     
==========================================
  Files         312      312              
  Lines       16412    16580     +168     
  Branches     4760     4817      +57     
==========================================
+ Hits        11314    11407      +93     
- Misses       5071     5146      +75     
  Partials       27       27              
Impacted Files Coverage Δ
packages/jest-resolve/src/resolver.ts 52.17% <45.61%> (-3.70%) ⬇️
packages/jest-resolve/src/defaultResolver.ts 87.64% <87.50%> (-1.04%) ⬇️
packages/expect/src/utils.ts 96.13% <0.00%> (+0.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7f0975...e58d96b. Read the comment docs.

@IanVS IanVS force-pushed the 9505-jest-resolve-async branch 3 times, most recently from 523cb92 to 28c133e Compare June 8, 2021 19:53
@IanVS IanVS changed the title WIP Add async defaultResolver to jest-resolve [jest-resolve] Add async resolver support Jun 9, 2021
@IanVS IanVS marked this pull request as ready for review June 9, 2021 02:37
@IanVS
Copy link
Contributor Author

IanVS commented Jun 14, 2021

@SimenB, do you have any chance to look over this? I know it's a large PR, and I'd be happy to simplify it if you can think of any ways to do so.

@geekox86
Copy link

Greetings. Guys what is the status on this important PR?

@SimenB
Copy link
Member

SimenB commented Aug 13, 2021

Hello! Thank you so much for tackling this issue!

Back from vacation now. I'll try to give this a look over the weekend (or next week), but no promises 🙂

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

(quick look)

packages/jest-resolve/src/defaultResolver.ts Outdated Show resolved Hide resolved
packages/jest-resolve/src/defaultResolver.ts Outdated Show resolved Hide resolved
packages/jest-resolve/src/defaultResolver.ts Outdated Show resolved Hide resolved
packages/jest-resolve/src/resolver.ts Show resolved Hide resolved
packages/jest-resolve/src/resolver.ts Outdated Show resolved Hide resolved
packages/jest-resolve/src/types.ts Outdated Show resolved Hide resolved
@IanVS
Copy link
Contributor Author

IanVS commented Aug 16, 2021

Thanks for the comments, @SimenB. I hope you enjoyed your vacation! I'll do my best to make changes this week or next weekend. I was without power for a few days last week after a big storm, so I'm still catching up on lost time.

@IanVS
Copy link
Contributor Author

IanVS commented Aug 31, 2021

I've rebased on latest master, and recombined the classes together. I apologize if there's too much churn in the diff, I still had to keep some methods broken up, and I arranged them in a way that made sense to me and helped me keep track of everything, but moving things around may have caused some churn.

@IanVS
Copy link
Contributor Author

IanVS commented Sep 9, 2021

@SimenB, is this looking closer to what you want? If so, I will try to keep it rebased on master.

@SimenB
Copy link
Member

SimenB commented Feb 17, 2022

Hopefully you haven't started to resolve the conflicts as #12373 is about to basically rewrite the default resolver 🙈

This is due to a bug, so hopefully there are no more bug reports before we get to this PR!

@IanVS
Copy link
Contributor Author

IanVS commented Feb 17, 2022

I haven't started yet, though was planning to this weekend. Procrastination pays off yet again!

Thanks for the heads up.

@IanVS
Copy link
Contributor Author

IanVS commented Feb 22, 2022

I took a shot at this and I'll be honest, I'm not sure I am the best one to finish this up. It's been so long since I did this work, I've forgotten what I did, and the new code looks pretty different than before. I've also moved on from trying to use Jest in my own project, so unfortunately I have no personal stake in this work either. I'd be perfectly happy if someone else wants to take this across the finish line, or maybe it just needs to be re-started from scratch, I'm not sure. But I can't really spend any more time on it now, sorry.

@SimenB
Copy link
Member

SimenB commented Feb 22, 2022

That's perfectly fine @IanVS, thanks! I'll give this a rebase 🙂

@SimenB
Copy link
Member

SimenB commented Feb 22, 2022

Ok, conflicts resolved. I skipped adding support for exports in async default resolver as I think I'll just remove async support for it (i.e. just delegate to sync). This is the same we did for transformation - while the runtime supports async transform, we don't actually use it ourselves as sync can always be wrapped in a promise

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

the code turned out quite dirty, this really needs a good refactor at some point (both runtime and resolver).

However, tests are happy, so let's try with this and hope people test it and report any bugs 🙂

Thank you so much for your patience @IanVS!

@SimenB SimenB merged commit 2d0496b into jestjs:main Feb 22, 2022
@IanVS IanVS deleted the 9505-jest-resolve-async branch March 18, 2022 14:40
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement async dependency resolution
6 participants