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

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

Merged
merged 1 commit into from
May 17, 2022

Conversation

F3n67u
Copy link
Contributor

@F3n67u F3n67u commented May 3, 2022

Follow-up of #2412

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

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

@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 3, 2022
@F3n67u
Copy link
Contributor Author

F3n67u commented May 4, 2022

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

ok.

@github-actions github-actions bot removed the needs-response We need a response from the original author about this issue/PR label May 4, 2022
@MichaelDeBoey MichaelDeBoey added the needs-response We need a response from the original author about this issue/PR label May 4, 2022
@F3n67u
Copy link
Contributor Author

F3n67u commented May 4, 2022

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

  1. <workspace-root>/packages/remix-dev/node_modules/tsconfig-paths: v4.0.0
  2. <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

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

F3n67u commented May 4, 2022

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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

@F3n67u
Copy link
Contributor Author

F3n67u commented May 4, 2022

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
Copy link
Contributor Author

F3n67u commented May 4, 2022

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

  1. <workspace-root>/packages/remix-dev/node_modules/tsconfig-paths: v4.0.0
  2. <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
Copy link
Member

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
Copy link
Member

Oh wow

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented May 16, 2022

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
Copy link
Member

@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 MichaelDeBoey marked this pull request as ready for review May 16, 2022 18:24
@MichaelDeBoey MichaelDeBoey requested a review from mcansh May 16, 2022 18:37
@F3n67u
Copy link
Contributor Author

F3n67u commented May 17, 2022

@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.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

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

@MichaelDeBoey
Copy link
Member

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

@kentcdodds kentcdodds merged commit be4a25e into remix-run:dev May 17, 2022
@kentcdodds
Copy link
Member

Thanks!

@F3n67u F3n67u deleted the jsconfig branch May 17, 2022 16:02
@MichaelDeBoey
Copy link
Member

@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
Copy link
Contributor

🤖 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
Copy link
Contributor Author

F3n67u commented May 18, 2022

@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
Copy link
Contributor

🤖 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
Copy link
Contributor Author

F3n67u commented May 19, 2022

@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
Copy link
Member

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

@kentcdodds
Copy link
Member

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
Copy link
Member

MichaelDeBoey commented May 19, 2022

@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
Copy link
Member

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

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented May 19, 2022

@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
Copy link
Member

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
Copy link

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.

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