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

Add cwd option to fix loader + jestTransformer when using moved cache + config options #626

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

comp615
Copy link
Contributor

@comp615 comp615 commented May 5, 2022

Addresses #605

Adds a new cwd option to the config and pipes it through path resolutions. There's a number of folder and base folders potentially involved in the process of running graphql-let: process dir, config dir, and other dirs.

In cases where the config file does not reside in the jest root directory. This is necessary because the config file can then give the cache location, and it will be resolved relative to its own location (not that of the jest config). Additionally, documents CANNOT reside outside the root directory. Since we store them in the cache dir as a literal directory, relative paths escaping that store the cache files in non-sensical places, additionally, this would imply we could have folder conflicts. This change helps resolve that ambiguity.

Put differently, now by default all paths will resolve relative to the config dir itself. But if you have the config dir in a sub-directory, this allows you to "reset" the base resolution directory opening up a wider variety of configuration options regardless of where you run jest from, store your config, etc.

Changes:

  • Adds new cwd option
  • Adds documentation around the new option
  • Returns final cwd from processed config
  • Updates execContext to return that config.cwd value which should update all resolutions
  • Updates jestTransformer to use this value (instead of jest rootDir once we resolve the config)
  • Updates loader to do the same
  • Adds jestTransformer test
  • Updates loader test to fix it not actually using the temp dir and to exercise the new cwd option

Partially addresses piglovesyou#605 

Sets a new cwd context in cases where the config file does not reside in the jest root directory. This is necessary because the config file can then give the cache location, but it will be relative to its own location (not that of the jest config)
@comp615 comp615 changed the title Fix jestTransformer for moved cache + config firs Fix jestTransformer when using moved cache + config options May 5, 2022
@piglovesyou
Copy link
Owner

Looks good, thank you @comp615! I'm ready to merge and publish once you fix the build failure, seemingly of a simple prettier error. Thanks!

@comp615
Copy link
Contributor Author

comp615 commented May 6, 2022

Looks good, thank you @comp615! I'm ready to merge and publish once you fix the build failure, seemingly of a simple prettier error. Thanks!

Thanks, will update, would also like to take a look at the test and see if I can add a case

@comp615 comp615 closed this May 6, 2022
@comp615 comp615 reopened this May 6, 2022
@comp615
Copy link
Contributor Author

comp615 commented May 9, 2022

All set now I think, wrote a new test!

@comp615
Copy link
Contributor Author

comp615 commented Jun 24, 2022

@piglovesyou wanted to flag this has been fixed and added another small PR for some security fixes. Thanks! We're enjoying it so far.

@comp615
Copy link
Contributor Author

comp615 commented Aug 31, 2022

NOTE: This same technique might need to be applied in more places like https://github.com/piglovesyou/graphql-let/blob/main/src/loader.ts, we continue to encounter this issue in various ways

@comp615
Copy link
Contributor Author

comp615 commented Apr 7, 2023

@piglovesyou Wanted to check in and see if you were still around! I've opened a few more PRs and would love to get them in (along with some of the dependabot stuff). If you are swamped, I'd also be happy to take a go at doing some maintainer work here and on NPM if you're willing. Happy to setup time to talk more if that's useful.

@comp615 comp615 changed the title Fix jestTransformer when using moved cache + config options Add cwd option to fix loader + jestTransformer when using moved cache + config options Jun 14, 2023
@comp615
Copy link
Contributor Author

comp615 commented Jun 14, 2023

Have updated this with a more complete solution.

- [FAQ](#faq)
- [Contribution](#contribution)
- [License](#license)
- [Why this exists](#why-this-exists)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this file is Prettier formatting this

@comp615
Copy link
Contributor Author

comp615 commented Oct 23, 2023

@piglovesyou saw you were doing some maintenance last week. Any chance of getting this one in? This is the most critical change in my mind of the ones open. We're currently running on a fork based off this branch in order to allow us to run a monorepo environment.

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.

2 participants