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

Fix incorrect resolution with relative imports on project dependencies #85

Merged
merged 3 commits into from Nov 13, 2021

Conversation

voliva
Copy link
Contributor

@voliva voliva commented Oct 13, 2021

I ran across #83 on my project, where I have a dependency which has a relative import to a file that happens to have the same name as one of my roots.

I think the issue is on this line: It's checking for the resolved innerRequest, which strips the relative prefix for dependencies. I think that what it should be is if the original non-resolved request (request.request).

With this change I no longer have this issue, but I'm not sure this is OK

@Rush
Copy link
Contributor

Rush commented Oct 14, 2021

rush@rushbox: /c/tsconfig-paths-webpack-plugin (relative-paths) 1.08s » yarn build
yarn run v1.22.11
warning ../package.json: No license field
$ rimraf lib && tsc -p src/tsconfig.lib.json
src/plugin.ts:248:7 - error TS2532: Object is possibly 'undefined'.

248       request.request.startsWith(".") ||
          ~~~~~~~~~~~~~~~

src/plugin.ts:249:7 - error TS2532: Object is possibly 'undefined'.

249       request.request.startsWith("..")
          ~~~~~~~~~~~~~~~

@Rush
Copy link
Contributor

Rush commented Oct 14, 2021

Please see voliva#1

@jonaskello
Copy link
Member

@voliva @Rush Do you think this is tested enough to be merged?

@Rush
Copy link
Contributor

Rush commented Oct 14, 2021

@jonaskello I am sorry but I do not have a well enough understanding of the project to say what was the intention of innerRequest and why changing it to request.request fixes the issue.

Also there aren't really any unit tests to tell us if anything regressed. I guess testing this is practice is the best bet. ;)

I hope @voliva can chime in as he did the original change and I merely made it publishable.

@jonaskello
Copy link
Member

Ok, so we'll wait for now. If someone wants to contribute some unit tests that would of course be very helpful :-).

@voliva
Copy link
Contributor Author

voliva commented Oct 14, 2021

@jonaskello I am sorry but I do not have a well enough understanding of the project to say what was the intention of innerRequest and why changing it to request.request fixes the issue.

Also there aren't really any unit tests to tell us if anything regressed. I guess testing this is practice is the best bet. ;)

I hope @voliva can chime in as he did the original change and I merely made it publishable.

I'm in a similar situation - I ran across the issue, tried to debug it and I saw:

  • innerRequest empirically gets a relative path without ./ prefix on dependencies.
  • There was no occurrence where innerRequest would start with . and request.request didn't.
  • Using the original request.request solved the issue and apparently no other issues surfaced.

But I'm not that familiar with the code or what each of these values mean, so I can't know for sure either if this is a good solution.

@cosmin-dev
Copy link

Can we merge this? I have the same issue

@Rush
Copy link
Contributor

Rush commented Nov 12, 2021

I've been using the forked version with the fix and it's been working flawlessly.

@voliva
Copy link
Contributor Author

voliva commented Nov 13, 2021

@jonaskello I'd say it's ready then :)

@jonaskello
Copy link
Member

Ok, let's merge this then :-)

@jonaskello jonaskello merged commit 2d838a0 into dividab:master Nov 13, 2021
@jonaskello
Copy link
Member

Released in 3.5.2

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.

None yet

4 participants