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

feat(remix-dev): add tsconfigPath config option to RemixConfig #3936

Merged
merged 5 commits into from Aug 25, 2022

Conversation

jwnx
Copy link
Contributor

@jwnx jwnx commented Aug 5, 2022

This patch addresses an issue[1] in which Remix looks for a tsconfig
outside the root directory.

We fix this behavior by looking for a tsconfig where it should be
(in the root directory itself) and passing its absolute path to
tsconfig-paths. tsconfig-paths will then treat the given path as a
tsconfig file path and won't crawl further. If no tsconfig file is
found by us in our root directory, we simply won't call tsconfig-paths
at all. Note that crawling up is tsconfig-paths intended behavior,
but not Remix's.

A secondary bug that causes this same issue is due to esbuild[2]. When
no tsconfig is passed to esbuild, it will crawl the filesystem up looking
for any tsconfig available and will load it. When tsconfig is explicitly
set to undefined, it does the crawling anyways. Unfortunately,
this is a fix that has to be done in esbuild, but a few alternatives
to manage this unwanted behavior for now can be:

  1. Create an empty tsconfig in the build directory and pass it to esbuild
    so it doesn't crawl outside rootDir;
  2. Show a warning to let the user know why the crawling is happening.

Closes #3210

[1] #3210
[2] evanw/esbuild#2440

Signed-off-by: Juliana Oliveira juliana@uma.ni

This patch addresses an issue[1] in which Remix looks for a tsconfig
outside the root directory.

We fix this behavior by looking for a tsconfig where it should be
(in the rootDirectory itself) and passing its absolute path to
`tsconfig-paths`. `tsconfig-paths` will then treat the given path as a
tsconfig file path and won't crawl further. If no tsconfig file is
found by us in our root directory, we simply won't call `tsconfig-paths`
at all. Note that crawling up is `tsconfig-path`s intended behavior,
but not Remix's.

A secondary bug that causes this same issue is due to esbuild[2]. When
no tsconfig is passed to esbuild, it will crawl the filesystem up looking
for any tsconfig available and will load it. When tsconfig is explicitly
set to `undefined`, it does the crawling anyways. Unfortunately,
this is a fix that has to be done in `esbuild`, but a few alternatives
to manage this unwanted behavior for now can be:

1. Create an empty tsconfig in the build directory and pass it to esbuild
   so it doesn't crawl outside rootDir;
2. Show a warning to let the user know why the crawling is happening.

Fixes remix-run#3210

[1] remix-run#3210
[2] evanw/esbuild#2440

Signed-off-by: Juliana Oliveira <juliana@uma.ni>
@changeset-bot
Copy link

changeset-bot bot commented Aug 5, 2022

⚠️ No Changeset found

Latest commit: 5edb557

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jwnx
Copy link
Contributor Author

jwnx commented Aug 8, 2022

(dev is already failing for windows unit and integration tests)

@MichaelDeBoey MichaelDeBoey changed the title fix(remix-dev): don't look for a tsconfig outside rootDir fix(remix-dev): add tsconfigPath config option Aug 8, 2022
@@ -450,6 +455,15 @@ export async function readConfig(

let serverDependenciesToBundle = appConfig.serverDependenciesToBundle || [];

// When tsconfigPath is undefined, the default "tsconfig.json" is not
// found in the root directory.
let tsconfigDefaultFilename = "tsconfig.json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you happen to know if this all works as expected if you use a jsconfig.json file and pass that to esbuild?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it appears that it does, can you update this to also check for jsconfig.json? i think something like this should be sufficient https://github.com/remix-run/remix/pull/3012/files#diff-852e3f3cc491a6fcc66706c94417be59f4c12569e0f367354613bfc103df7b38R299-R307

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. There you go!

Copy link
Collaborator

Choose a reason for hiding this comment

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

:chefkiss:

Now we not only consider tsconfig.json, but also `jsconfig.json`.

Signed-off-by: Juliana Oliveira <juliana@uma.ni>
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Aug 22, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@jwnx
Copy link
Contributor Author

jwnx commented Aug 25, 2022

@mcansh I can't merge this. How is this done?

@mcansh
Copy link
Collaborator

mcansh commented Aug 25, 2022

@mcansh I can't merge this. How is this done?

getting this over the finish line now :)

going to update the pr with dev to make sure everything is still swimming along

@mcansh mcansh changed the title fix(remix-dev): add tsconfigPath config option feat(remix-dev): add tsconfigPath config option to RemixConfig Aug 25, 2022
@mcansh mcansh merged commit a50246d into remix-run:dev Aug 25, 2022
@MichaelDeBoey MichaelDeBoey added the awaiting release This issue has been fixed and will be released soon label Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed NEW API package:dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build fails if there's a broken tsconfig.json file outside the project
3 participants