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.

Sign up for GitHub

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

Jump to bottom

fix: tsconfig paths are not expanded correctly for tsconfigs and libs not placed at workspace root (#499) #500

Closed
wants to merge 4 commits into from
Closed

fix: tsconfig paths are not expanded correctly for tsconfigs and libs not placed at workspace root (#499) #500

wants to merge 4 commits into from

Conversation

joelMuehlena
Copy link
Contributor

@joelMuehlena joelMuehlena commented Aug 5, 2023 •
edited
Loading

Fix #499

Test plan

  • Covered by existing test cases
  • New test cases added

General

This fix ensures, that if an expanded import path like specified in the corresponding tsconfig.json (of a file which for example lays in the following space /projects/p1) is not absolute e.g. @org/liba/* -> ./*: @org/liba/test will be expanded to projects/p1/test. Otherwise gazelle has issues resolving the correct imports.

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@joelMuehlena joelMuehlena changed the title fix: tsconfig paths are not expanded correctly for tsconfigs and libs not placed at workspace root fix: tsconfig paths are not expanded correctly for tsconfigs and libs not placed at workspace root (#499) Aug 5, 2023
@xc2
Copy link

xc2 commented Aug 7, 2023 •
edited
Loading

Hello @joelMuehlena,

Thanks for your help with relative imports. They're working great with baseUrl and paths, but I'm running into an issue where they don't work with just baseUrl alone. Here's a codesandbox with the issue (basic ts setup without bazel): https://codesandbox.io/s/practical-architecture-wlft6z?file=/src/App.tsx:0-31

Could you please add support for baseUrl-alone as well? That would be a huge help!

Thanks again

@joelMuehlena
Copy link
Contributor Author

Hi @xc2,
I looked into your codesandbox & request and made some changes. Maybe you can have a look if those changes are reflecting your request.

@joelMuehlena
Copy link
Contributor Author

@jbedard this should be everything. Maybe you can have a look at it, when you have the time?

jbedard
jbedard reviewed Aug 9, 2023
View reviewed changes
gazelle/js/typescript/tsconfig.go Outdated
@@ -99,6 +118,8 @@ func parseTsConfigJSON(cm *TsConfigMap, root, configDir string, tsconfigContent
var BaseUrl string
if c.CompilerOptions.BaseUrl != nil {
BaseUrl = path.Clean(*c.CompilerOptions.BaseUrl)
} else if baseConfig != nil && baseConfig.BaseUrl != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Does one of your tests fail without this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but it handles the case, if a config was extended with a baseUrl and the local one not having one the extended one will be used. It's quite difficult to test this case without "a real file", because the parsing function loads the extension of a file directly. So we can remove this but then the case with a baseDir inherited from an extension file will not be covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should I handle this? Just leave it as is?

@jbedard
Copy link
Member

jbedard commented Aug 9, 2023

A bunch of minor comments but overall looks great! Could merge as-is but if you have time to address or reply to the comments that would be great 👍

@joelMuehlena
Copy link
Contributor Author

Yep no problem, I'll take a look on the review this week when I have some time.

@xc2
Copy link

xc2 commented Aug 10, 2023

Hi @xc2, I looked into your codesandbox & request and made some changes. Maybe you can have a look if those changes are reflecting your request.

Thank you, it works as expected!

@joelMuehlena joelMuehlena force-pushed the fix-499-tsconfig-rel-paths branch from 9f8aa4e to d3abd3f Compare August 13, 2023 10:11
jbedard
jbedard approved these changes Aug 14, 2023
View reviewed changes
Copy link
Member

@jbedard jbedard left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @joelMuehlena

@alexeagle alexeagle force-pushed the fix-499-tsconfig-rel-paths branch from d3abd3f to 19558e3 Compare August 14, 2023 20:43
@alexeagle alexeagle closed this Aug 14, 2023
alexeagle added a commit that referenced this pull request Aug 14, 2023
@alexeagle @joelMuehlena
chore(copybara): import aspect-cli#500 (#3166)
073dbc1 
Synced from #500 by
joelMuehlena:
fix: tsconfig paths are not expanded correctly for tsconfigs and libs
not placed at workspace root (#499)

---

Fix #499

### Test plan

- Covered by existing test cases
- New test cases added

### General

This fix ensures, that if an expanded import path like specified in the
corresponding tsconfig.json (of a file which for example lays in the
following space <root>/projects/p1) is not absolute e.g. `@org/liba/* ->
./*: @org/liba/test` will be expanded to `projects/p1/test`. Otherwise
gazelle has issues resolving the correct imports.

Closes #500

---------

Co-authored-by: Joel Mühlena <[email protected]>
GitOrigin-RevId: 57f6b0b18df932cc543d334b4eac1118e70674cf
@alexeagle
Copy link
Member

Thanks, we brought the PR into our monorepo and exporting again in #502

gregmagolan pushed a commit that referenced this pull request Aug 24, 2023
@alexeagle @joelMuehlena @gregmagolan
chore(copybara): import aspect-cli#500 (#3166)
ad1d4d6 
Synced from #500 by
joelMuehlena:
fix: tsconfig paths are not expanded correctly for tsconfigs and libs
not placed at workspace root (#499)

---

Fix #499

### Test plan

- Covered by existing test cases
- New test cases added

### General

This fix ensures, that if an expanded import path like specified in the
corresponding tsconfig.json (of a file which for example lays in the
following space <root>/projects/p1) is not absolute e.g. `@org/liba/* ->
./*: @org/liba/test` will be expanded to `projects/p1/test`. Otherwise
gazelle has issues resolving the correct imports.

Closes #500

---------

Co-authored-by: Joel Mühlena <[email protected]>
GitOrigin-RevId: 57f6b0b18df932cc543d334b4eac1118e70674cf
alexeagle added a commit that referenced this pull request Aug 31, 2023
@alexeagle @joelMuehlena
chore(copybara): import aspect-cli#500 (#3166)
550066e 
Synced from #500 by
joelMuehlena:
fix: tsconfig paths are not expanded correctly for tsconfigs and libs
not placed at workspace root (#499)

---

Fix #499

### Test plan

- Covered by existing test cases
- New test cases added

### General

This fix ensures, that if an expanded import path like specified in the
corresponding tsconfig.json (of a file which for example lays in the
following space <root>/projects/p1) is not absolute e.g. `@org/liba/* ->
./*: @org/liba/test` will be expanded to `projects/p1/test`. Otherwise
gazelle has issues resolving the correct imports.

Closes #500

---------

Co-authored-by: Joel Mühlena <[email protected]>
GitOrigin-RevId: 57f6b0b18df932cc543d334b4eac1118e70674cf
alexeagle added a commit that referenced this pull request Aug 31, 2023
@alexeagle @joelMuehlena
chore(copybara): import aspect-cli#500 (#3166)
972e2aa 
Synced from #500 by
joelMuehlena:
fix: tsconfig paths are not expanded correctly for tsconfigs and libs
not placed at workspace root (#499)

---

Fix #499

### Test plan

- Covered by existing test cases
- New test cases added

### General

This fix ensures, that if an expanded import path like specified in the
corresponding tsconfig.json (of a file which for example lays in the
following space <root>/projects/p1) is not absolute e.g. `@org/liba/* ->
./*: @org/liba/test` will be expanded to `projects/p1/test`. Otherwise
gazelle has issues resolving the correct imports.

Closes #500

---------

Co-authored-by: Joel Mühlena <[email protected]>
GitOrigin-RevId: 57f6b0b18df932cc543d334b4eac1118e70674cf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbedard jbedard jbedard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: tsconfig paths are not expanded correctly for relative paths
5 participants
@joelMuehlena @CLAassistant @xc2 @jbedard @alexeagle