analog icon indicating copy to clipboard operation
analog copied to clipboard

feat(vite-plugin-nitro): add support for typescript paths import resolution

Open jeremyhofer opened this issue 1 year ago • 9 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [X] The commit message follows our guidelines: https://github.com/analogjs/analog/blob/main/CONTRIBUTING.md#-commit-message-guidelines
  • [] Tests for the changes have been added (for bug fixes / features)
  • [] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [ ] Bugfix
  • [X] Feature
  • [ ] Code style update (formatting, local variables)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Documentation content changes
  • [ ] Other... Please describe:

Which package are you modifying?

  • [ ] vite-plugin-angular
  • [X] vite-plugin-nitro
  • [ ] astro-angular
  • [ ] create-analog
  • [ ] router
  • [X] platform
  • [ ] content
  • [X] nx-plugin
  • [ ] trpc

What is the current behavior?

Currently typescript imports using the compilerOptions.paths aliases set in tsconfig.json are not automatically handled for Nitro. This causes the nitro server to fail to run when executing the imports. This is especially evident in a Nx monorepo leveraging multiple in-built libraries.

Closes # Discord Thread

What is the new behavior?

This PR adds rollup-plugin-typescript-paths as a dependency of vite-plugin-nitro. This plugin is included as one of the plugins within the rollupConfig of the NitroConfig.

The plugin is configured to default to loading / reading from a tsconfig.json file in the root of the repo. However, to support other setups an option was added to both the Options for vite-plugin-nitro and the @analog/platform plugin to allow a user to override the tsconfig file / path.

Related to the 2nd point, the nx-plugin was update to specify tsconfig.base.json as the tsconfig file to load. This was added as a default for all current vite.config.ts templates in the plugin.

Does this PR introduce a breaking change?

  • [ ] Yes
  • [X] No

Other information

[optional] What gif best describes this PR or how it makes you feel?

jeremyhofer avatar Dec 12 '23 00:12 jeremyhofer

Deploy Preview for analog-docs ready!

Name Link
Latest commit 6edaeba403c6370891daf807d6a950ceb6d7d83f
Latest deploy log https://app.netlify.com/sites/analog-docs/deploys/6577a7106c6a210008136a9d
Deploy Preview https://deploy-preview-792--analog-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Dec 12 '23 00:12 netlify[bot]

Deploy Preview for analog-blog ready!

Name Link
Latest commit 6edaeba403c6370891daf807d6a950ceb6d7d83f
Latest deploy log https://app.netlify.com/sites/analog-blog/deploys/6577a7105793ce0008f0eaf9
Deploy Preview https://deploy-preview-792--analog-blog.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Dec 12 '23 00:12 netlify[bot]

Deploy Preview for analog-app ready!

Name Link
Latest commit 6edaeba403c6370891daf807d6a950ceb6d7d83f
Latest deploy log https://app.netlify.com/sites/analog-app/deploys/6577a710fe3ce200087eb023
Deploy Preview https://deploy-preview-792--analog-app.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Dec 12 '23 00:12 netlify[bot]

@brandonroberts wanted to follow up on this PR. Anything further I should look to address?

jeremyhofer avatar Dec 18 '23 04:12 jeremyhofer

@jeremyhofer looks good in general. Just a little stuck on the naming because we already have a tsconfig property the vite object, which is different from this one and may cause some confusion.

analog({
  vite: { tsconfig: 'path/to/tsconfig.json' },
  tsConfigPath: 'tsconfig.base.json'
})

Maybe it should be workspaceTsConfig to go along with workspaceRoot.

Thoughts?

brandonroberts avatar Dec 19 '23 15:12 brandonroberts

@brandonroberts been experimenting with this and thinking more about how to best handle this, and want your thoughts.

First, some additional things I've determined regarding tsconfig paths support in analog today:

  1. The import issue the PR is currently targeting only impacts running the Nitro server (run, serve, build and execute, and similar targets). It specifically does not have an impact on the test target.
  2. Using tsconfig path imports within components / pages and unit tests cannot resolve imports either. However, this is resolved via vite-tsconfig-paths instead of the rollup/nitro plugin. Nx has migrations that checks for and adds vite-tsconfig-paths to your vite-config.ts file if it is not present, so I was not seeing the issue in my repo. However, removing that plugin caused the paths to not be resolved. Similarly, in a new standlone analog app, if you configure paths in tsconfig.json they are not recognized.

Regarding the current approach in this PR targeting a fix for the Nitro server - I think we can get by without configuring a tsconfig file, instead just referencing tsconfig.app.json. This ultimately extends the root tsconfig.json in a standalone analog project, or the tsconfig.base.json in a Nx repo. Unfortunately the rollup-plugin-typescript-paths dependency I added does not use typescript to fully resolve the tsconfig, so extends inheritance is not respected. This will have to be manually tooled.

I'm not having luck finding a rollup plugin that handles the tsconfig paths in a way that works with analog. Rollup provides the @rollup/plugin-typescript, but it requires specific tsconfig configurations, especially related to the outDir, that do not work with how analog bundles the application. Other plugins similarly either are doing too much or not enough to fit the analog usecase.

Given all the above, I'm currently considering the following:

  1. Manually implementing a rollup plugin to handle the tsconfig paths. Functionally it be like rollup-plugin-typescript-paths, but will use the typescript compiler to resolve the tsconfig and handle all inheritance, as the @rollup/plugin-typescript plugin does. This plugin will reference tsconfig.app.json.
  2. Adding vite-tsconfig-paths as a dependency of @analogjs/platform and including the plugin in the set of plugins provided via the platform plugin, as this is needed for all project targets.

Thoughts or concerns on any of the above?

jeremyhofer avatar Dec 27 '23 18:12 jeremyhofer

Option 2 is a better solution from a long-term maintenance standpoint. Nx uses their own tsconfig paths plugin, will this cause any conflicts with that?

brandonroberts avatar Dec 30 '23 03:12 brandonroberts

In this case what I recommended weren't different options, I'd have to do both items 1 and 2 from the bottom of my last comment to resolve tsconfig path support for all use cases.

For item 1 I called out the question is around if we're ok implementing the rollup plugin to support tsconfig paths for nitro / rollup, which is needed for bundling / running the server. Alternative would be to work the @rollup/plugin-typescript plugin into vite-plugin-nitro, but this may drive refactoring in vite-plugin-nitro.

For item 2, good callout on Nx's tsconfig path plugin. I just saw that this evening while experimenting with my test related posts. In my current project Nx had added the vite-tsconfig-paths plugin and configured it in a migration, their own plugin wasn't introduced yet. I'll experiment with this and see if there's any issues with analog including a vite tsconfig path plugin by default.

jeremyhofer avatar Dec 30 '23 03:12 jeremyhofer

I've seen a couple others run into this also, but I think we would be better off adding this as a section in the Nx guide where its most likely to happen.

https://analogjs.org/docs/integrations/nx

Thoughts?

brandonroberts avatar Mar 11 '24 19:03 brandonroberts