refactor(remix-dev): clean up custom `tsconfig` loader by F3n67u · Pull Request #3074 · remix-run/remix

F3n67u · 2022-05-03T15:19:17Z

Follow-up of #2412

MichaelDeBoey · 2022-05-03T16:59:14Z

@F3n67u Please look into why the integration test are failing.

F3n67u · 2022-05-04T11:49:14Z

@F3n67u Please look into why the integration test are failing.

ok.

F3n67u · 2022-05-04T13:47:15Z

After I bump @remix-run/dev's tsconfig-paths version to ^4.0.0, there will exist two different versions on node_modules.

<workspace-root>/packages/remix-dev/node_modules/tsconfig-paths: v4.0.0
<workspace-root>/node_modules/tsconfig-paths: v3.14.1, introduced by eslint-plugin-import and eslint-import-resolver-typescript

When running yarn build, <workspace-root>/packages/remix-dev/node_modules is not copied to build/node_modules/@remix-run/dev. so when running test the tsconfig-paths deps is resolved to <workspace-root>/node_modules/tsconfig-paths(v3.14.1) instead of <workspace-root>/packages/remix-dev/node_modules/tsconfig-paths(v4.0.0), this is why the test is failed.

I don't know how to solve this problem, any thought? @MichaelDeBoey

here is how to reproduce this resolve problem.

# git checkout <this-pr>
$ yarn
$ yarn build
$ touch build/node_modules/@remix-run/dev/test.js
$ echo "console.log(require.resolve('tsconfig-paths'))" >> build/node_modules/@remix-run/dev/test.js
$ node build/node_modules/@remix-run/dev/test.js                                                    
/Users/feng/dev/remix/node_modules/tsconfig-paths/lib/index.js

$ npm list tsconfig-paths                       
remix-monorepo@ /Users/feng/dev/remix
├─┬ @remix-run/dev@1.4.3 -> ./packages/remix-dev
│ └── tsconfig-paths@4.0.0
└─┬ @remix-run/eslint-config@1.4.3 -> ./packages/remix-eslint-config
  ├─┬ eslint-import-resolver-typescript@2.5.0
  │ └── tsconfig-paths@3.14.1
  └─┬ eslint-plugin-import@2.25.4
    └── tsconfig-paths@3.14.1 deduped

F3n67u · 2022-05-04T14:16:08Z

I make pr to bump tsconfig-paths version to v4 on import-js/eslint-plugin-import#2447 and import-js/eslint-import-resolver-typescript#104. If our upstream deps all bump their tsconfig-paths to v4, then the problem will be solved either.

MichaelDeBoey · 2022-05-04T14:19:52Z

@F3n67u Having eslint-import-resolver-typescript & eslint-plugin-import update their dependency on tsconfig-paths would be the easiest solution.

If they don't want to (because of loss of Node v4 compatibility) or it takes too long, maybe @kentcdodds can help out with figuring out the best approach here.

kentcdodds · 2022-05-04T14:38:42Z

Well, if the latest version of tsconfig-paths requires Node v14 then we can't use it either because we still support Node v12 I believe 😬

kentcdodds · 2022-05-04T14:39:23Z

We can't drop support for v12 until our next major version.

F3n67u · 2022-05-04T15:23:53Z

Well, if the latest version of tsconfig-paths requires Node v14 then we can't use it either because we still support Node v12 I believe 😬

the latest tsconfig-paths just require node v6.😁

F3n67u · 2022-05-04T15:26:53Z

After I bump @remix-run/dev's tsconfig-paths version to ^4.0.0, there will exist two different versions on node_modules.

<workspace-root>/packages/remix-dev/node_modules/tsconfig-paths: v4.0.0

<workspace-root>/node_modules/tsconfig-paths: v3.14.1, introduced by eslint-plugin-import and eslint-import-resolver-typescript

When running yarn build, <workspace-root>/packages/remix-dev/node_modules is not copied to build/node_modules/@remix-run/dev. so when running test the tsconfig-paths deps is resolved to <workspace-root>/node_modules/tsconfig-paths(v3.14.1) instead of <workspace-root>/packages/remix-dev/node_modules/tsconfig-paths(v4.0.0), this is why the test is failed.

I don't know how to solve this problem, any thought? @MichaelDeBoey

here is how to reproduce this resolve problem.
# git checkout <this-pr>
$ yarn
$ yarn build
$ touch build/node_modules/@remix-run/dev/test.js
$ echo "console.log(require.resolve('tsconfig-paths'))" >> build/node_modules/@remix-run/dev/test.js
$ node build/node_modules/@remix-run/dev/test.js                                                    
/Users/feng/dev/remix/node_modules/tsconfig-paths/lib/index.js
$ npm list tsconfig-paths                       
remix-monorepo@ /Users/feng/dev/remix
├─┬ @remix-run/dev@1.4.3 -> ./packages/remix-dev
│ └── tsconfig-paths@4.0.0
└─┬ @remix-run/eslint-config@1.4.3 -> ./packages/remix-eslint-config
 ├─┬ eslint-import-resolver-typescript@2.5.0
 │ └── tsconfig-paths@3.14.1
 └─┬ eslint-plugin-import@2.25.4
   └── tsconfig-paths@3.14.1 deduped

@kentcdodds the real problem is current build dir cannot resolve a npm packages which is not hoisted to top node_modules dir.

MichaelDeBoey · 2022-05-04T15:42:44Z

Well, if the latest version of tsconfig-paths requires Node v14 then we can't use it either because we still support Node v12 I believe 😬

The minimum required Node version is v14 for Remix

But we're talking about these packages dropping v4 (yes 10! version below) here 🙈

kentcdodds · 2022-05-05T05:07:21Z

Oh wow

MichaelDeBoey · 2022-05-16T11:43:23Z

It doesn't seem like import-js/eslint-plugin-import#2447 is going to be merged (very soon).

@kentcdodds This however shouldn't matter though.
Code/tests shouldn't fail because 2 dependencies have a dependency on another version of the same package though 🤷‍♂️
So it might be worth it to look at fixing that instead of waiting for eslint-plugin-import to be updated.

As said by @F3n67u, this is caused by:

When running yarn build, <workspace-root>/packages/remix-dev/node_modules is not copied to build/node_modules/@remix-run/dev. so when running test the tsconfig-paths deps is resolved to <workspace-root>/node_modules/tsconfig-paths(v3.14.1) instead of <workspace-root>/packages/remix-dev/node_modules/tsconfig-paths(v4.0.0), this is why the test is failed.

MichaelDeBoey · 2022-05-16T18:24:50Z

@F3n67u @kentcdodds This problem seems to be fixed already without us knowing. 🤔

So I think we can just review the code & merge it if all is good.

F3n67u · 2022-05-17T00:08:39Z

@F3n67u @kentcdodds This problem seems to be fixed already without us knowing. 🤔

So I think we can just review the code & merge it if all is good.

wow. that's great.

kentcdodds

I love deleting code. Do we have tests for this? How was this change verified to have worked?

MichaelDeBoey · 2022-05-17T15:46:03Z

@kentcdodds Tests were introduced in #2412 and they're still passing 🥳🎉

kentcdodds · 2022-05-17T15:55:59Z

Thanks!

MichaelDeBoey · 2022-05-17T16:33:06Z

@F3n67u I just see that your tests created in #2412 doesn't include a jsconfig.json file.
Are you willing to include such a test please?
If not, I'm happy to do it myself, but since it's all your code/tests & it's probably an oversight, I wanted to ask you first.

github-actions · 2022-05-17T19:12:57Z

🤖 Hello there,

We just published version v1.5.0-pre.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

F3n67u · 2022-05-18T00:37:26Z

@F3n67u I just see that your tests created in #2412 doesn't include a jsconfig.json file.
Are you willing to include such a test please?
If not, I'm happy to do it myself, but since it's all your code/tests & it's probably an oversight, I wanted to ask you first.

@MichaelDeBoey good point. I will add a test for jsconfig.json later.

github-actions · 2022-05-18T20:05:54Z

🤖 Hello there,

We just published version v1.5.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

F3n67u · 2022-05-19T15:46:38Z

@F3n67u @kentcdodds This problem seems to be fixed already without us knowing. 🤔

So I think we can just review the code & merge it if all is good.

@MichaelDeBoey I think it's because we have not covered jsconfig.json scenario anymore, so the test passed even though the tsconfig-paths resolved to the v3 version which does not have jsconfig.json support when testing.

We do use jsconfig.json fixture when I created #2412, but not anymore at this time.

when I add a jsconfig.json integration test, the test starts failing because the resolved v3 tsconfig-paths doesn't support jsconfig.json.

MichaelDeBoey · 2022-05-19T16:12:19Z

@kentcdodds That brings us to the point I described in #3074 (comment) again.

kentcdodds · 2022-05-19T17:13:18Z

Sorry, I haven't taken the time to fully understand the constraints and issues here. Can you help me understand you suggest we do?

MichaelDeBoey · 2022-05-19T17:27:15Z

@kentcdodds As @F3n67u explained in #3074 (comment): the problem is that tests are failing because we are using the wrong version of a certain package.

In this specific case: we updated tsconfig-paths to v4 in @remix-run/dev, but when running tests we use v3 of that package.
So when running tests that target specific behavior of the updated package, they fail.
Even though the code in @remix-run/dev works like expected.

I think that should be fixed on our end somehow.

kentcdodds · 2022-05-19T17:40:20Z

I see. And why are we using v3 of that package in the tests?

MichaelDeBoey · 2022-05-19T17:50:40Z

@kentcdodds That's the actual question 🙈
Imo it should use v4, as @remix-run/dev is using that version of the package

According to @F3n67u's explanation, we're not copying over node_modules when running the build, which is a normal/expected behavior imo.
Our (integration) tests however use the local build files.
This means that npm will search for the nearest version of a certain package when needing to import it.

Since v4 is only used in @remix-run/dev and v3 is used by dependencies of other packages, v4 will be in packages/remix-dev/node_modules & not in the root's node_modules.
This has the implication that npm will find v3, so when we're using v4 specific API's/functionality, it will fail to do so.

kentcdodds · 2022-05-19T19:41:33Z

Oh, I see. Yeah the way we're managing node_modules for our integration tests and things is ... not the best. We're hoping to eventually move to Nx which would help with this I think. But until then... I guess we'll just not be able to have tests for this without a lot of work. I think it's probably not worth the effort. If we tested it manually and it worked, let's just be satisfied for that for now. Maybe we could write the tests and add a .skip on them with a // TODO: comment explaining why they're skipped for now.

01taylop · 2022-06-13T17:51:46Z

Hello, I stumbled here from the eslint-plugin-import issue which I also want resolved. Unfortunately I don't think this will happen any time soon but I had a similar hoisting issue with a Babel dependency not so long ago. I think you can get around the issue by switching back to npm or by upgrading to Yarn 2/3 (whichever it's called). Just wanted to share that in case it is a path you want to explore.

remix-cla-bot bot added the CLA Signed label May 3, 2022

MichaelDeBoey added the package:dev label May 3, 2022

MichaelDeBoey changed the title ~~refactor(remix-dev): clean up custom tsconfig loader~~ refactor(remix-dev): clean up custom tsconfig loader May 3, 2022

MichaelDeBoey force-pushed the jsconfig branch from a7e37b7 to 2b22b01 Compare May 3, 2022 16:55

MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 3, 2022

github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 4, 2022

MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 4, 2022

github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 4, 2022

MichaelDeBoey mentioned this pull request May 4, 2022

[Deps] upgrade tsconfig-paths to ^4.0.0 import-js/eslint-plugin-import#2447

Closed

refactor(remix-dev): clean up custom tsconfig loader

ae3435a

MichaelDeBoey force-pushed the jsconfig branch from 2b22b01 to ae3435a Compare May 16, 2022 11:45

MichaelDeBoey marked this pull request as ready for review May 16, 2022 18:24

MichaelDeBoey approved these changes May 16, 2022

View reviewed changes

MichaelDeBoey requested a review from mcansh May 16, 2022 18:37

kentcdodds approved these changes May 17, 2022

View reviewed changes

kentcdodds merged commit be4a25e into remix-run:dev May 17, 2022

F3n67u deleted the jsconfig branch May 17, 2022 16:02

F3n67u mentioned this pull request May 19, 2022

tests: add jsconfig path mapping integration test #3252

Closed

2 tasks

donavon mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#17

Open

snyk-bot mentioned this pull request Aug 1, 2022

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#19

Open

donavon mentioned this pull request Aug 1, 2022

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#20

Open

This was referenced Nov 28, 2023

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#34

Open

[Snyk] Security upgrade @remix-run/serve from 1.2.3 to 1.5.0 donavon/remix#36

Open

Conversation

F3n67u commented May 3, 2022 • edited by MichaelDeBoey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelDeBoey commented May 3, 2022

Uh oh!

F3n67u commented May 4, 2022

Uh oh!

F3n67u commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

here is how to reproduce this resolve problem.

Uh oh!

F3n67u commented May 4, 2022

Uh oh!

MichaelDeBoey commented May 4, 2022

Uh oh!

kentcdodds commented May 4, 2022

Uh oh!

kentcdodds commented May 4, 2022

Uh oh!

F3n67u commented May 4, 2022

Uh oh!

F3n67u commented May 4, 2022

here is how to reproduce this resolve problem.

Uh oh!

MichaelDeBoey commented May 4, 2022

Uh oh!

kentcdodds commented May 5, 2022

Uh oh!

MichaelDeBoey commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelDeBoey commented May 16, 2022

Uh oh!

F3n67u commented May 17, 2022

Uh oh!

kentcdodds left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelDeBoey commented May 17, 2022

Uh oh!

kentcdodds commented May 17, 2022

Uh oh!

MichaelDeBoey commented May 17, 2022

Uh oh!

github-actions bot commented May 17, 2022

Uh oh!

F3n67u commented May 18, 2022

Uh oh!

github-actions bot commented May 18, 2022

Uh oh!

F3n67u commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelDeBoey commented May 19, 2022

Uh oh!

kentcdodds commented May 19, 2022

Uh oh!

MichaelDeBoey commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentcdodds commented May 19, 2022

Uh oh!

MichaelDeBoey commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kentcdodds commented May 19, 2022

Uh oh!

01taylop commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

F3n67u commented May 3, 2022 •

edited by MichaelDeBoey

Loading

F3n67u commented May 4, 2022 •

edited

Loading

MichaelDeBoey commented May 16, 2022 •

edited

Loading

F3n67u commented May 19, 2022 •

edited

Loading

MichaelDeBoey commented May 19, 2022 •

edited

Loading

MichaelDeBoey commented May 19, 2022 •

edited

Loading