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

tests: add jsconfig path mapping integration test #3252

Closed
wants to merge 2 commits into from

Conversation

F3n67u
Copy link
Contributor

@F3n67u F3n67u commented May 19, 2022

Follow up of #3074 (comment)

  • Docs
  • Tests

Testing Strategy:

@F3n67u F3n67u changed the title test: add jsconfig path mapping integration test tests: add jsconfig path mapping integration test May 19, 2022
@MichaelDeBoey
Copy link
Member

@F3n67u As suggested by @kentcdodds in #3074 (comment):

[...] write the tests and add a .skip on them with a // TODO: comment explaining why they're skipped for now.

@@ -18,7 +18,7 @@ interface FixtureInit {
buildStdio?: Writable;
sourcemap?: boolean;
files: { [filename: string]: string };
template?: "cf-template" | "deno-template" | "node-template";
template?: "cf-template" | "deno-template" | "node-template" | "node-javascript-template";
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we want an extra template for this though. 🤔

Maybe we should wait for the convert-to-javascript migration (see #3150) & call the migration before we do anything else in our tests?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... I think I'd prefer to duplicate than make a dependency here.

Copy link
Member

Choose a reason for hiding this comment

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

This means we'll potentially have them double.

What about adding an extra (optional) key to FixtureInit that says if we want to use TypeScript or not.
If not, the createFixture function could run the migration automatically?
That way we don't need double templates + we can choose to use JS in our tests if we want to.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with that 👍

@chaance
Copy link
Collaborator

chaance commented Jan 24, 2023

I noticed this is still a draft PR, so I'm closing for now while we clean up some old issues/PRs. Feel free to ping me if and when you're ready to see this through!

@chaance chaance closed this Jan 24, 2023
@MichaelDeBoey
Copy link
Member

I think these tests are still valuable to have

@F3n67u Could you please open a new PR with these tests?

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

Successfully merging this pull request may close these issues.

4 participants