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

feat(vite): Use app/lib tsconfig for buildable paths resolution #19972

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

pulecka
Copy link
Contributor

@pulecka pulecka commented Nov 1, 2023

This change uses actual project TypeScript config - i.e. tsconfig.app.json or tsconfig.lib.json - to generate temporary tsconfig file when building with "buildLibsFromSource": false option.

This is needed when users would want to use the generated tsconfig in their Vite configuration - e.g. for generating type declarations.

Also exposed the generated tsconfig via NX_TSCONFIG_PATH env variable. This way users trying to use the generated config don't have to know & depend on specific path which feels like an internal implementation detail.

So now user can use vite-plugin-dts or @rollup/plugin-typescript to generate type declarations during build:

export default defineConfig({
  plugins: [
    nxViteTsPaths(),
    {
      ...typescript({
        tsconfig: process.env.NX_TSCONFIG_PATH ?? join(__dirname, 'tsconfig.lib.json'),
        noForceEmit: true,
      }),
      apply: 'build',
    },
  ],
})

Current Behavior

Generated temporary tsconfig file is based on project tsconfig.json file which is shared in typical NX setups for tests & build. And as such it is missing some options that are relevant only for build - e.g. emitDeclarationOnly.

@pulecka pulecka requested a review from a team as a code owner November 1, 2023 13:30
Copy link

vercel bot commented Nov 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview Dec 5, 2023 8:57am

@pulecka pulecka force-pushed the feat/vite-generated-lib-app-config branch from 14f1fe7 to 34bd768 Compare November 1, 2023 14:22
projectRoot,
dependencies
);
process.env.NX_TSCONFIG_PATH = tmpTsConfigPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

When options.buildLibsFromSource === true, process.env.NX_TSCONFIG_PATH would be undefined. Is that desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's expected - process.env.NX_TSCONFIG_PATH would also be undefined during test or serve in case of app.
so the consumer would have to always check it like:

const tsconfigPath = process.env.NX_TSCONFIG_PATH ?? join(__dirname, 'tsconfig.lib.json')

whether it's desired I'm not that certain - it seems to me to be better for consumers than doing something like:

const tmpTsconfigPath = join(
  workspaceRoot,
  'tmp',
  projectRootFromWorkspaceRoot,
  'tsconfig.generated.json'
)
const tsconfigPath = existsSync(tmpTsconfigPath) ? tmpTsconfigPath : join(__dirname, 'tsconfig.lib.json')

both of these are leaking implementation details, but the first one is a little less leaky imho

@belaczek
Copy link
Contributor

belaczek commented Dec 4, 2023

Can anyone please check this PR? It is currently blocking us from Vite adoption in our project.
CC @JamesHenry

@mandarini
Copy link
Member

@belaczek @pulecka thanks I'll take a look!

@mandarini mandarini self-assigned this Dec 4, 2023
Copy link
Member

@mandarini mandarini left a comment

Choose a reason for hiding this comment

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

Ok sure I'll approve this! Thanks for the PR!

@mandarini
Copy link
Member

mandarini commented Dec 4, 2023

@mandarini
Copy link
Member

@belaczek you either need to rebase with master to resolve conflicts, or give me push rights on your branch and I can do the rebase! then we can merge!

This change uses actual project TypeScript config - i.e.
`tsconfig.app.json` or `tsconfig.lib.json` - to generate temporary
`tsconfig` when building with `"buildLibsFromSource": false` option.

This is needed when users would want to use the generated `tsconfig` in
their Vite configuration - e.g. for generating type declarations.

Also exposed the generated `tsconfig` via `NX_TSCONFIG_PATH` env
variable. This way users trying to use the generated config don't have
to know & depend on specific path which feels like an internal
implementation detail.

So now user can use `vite-plugin-dts` or `@rollup/plugin-typescript` to
generate type declarations during build:

```
export default defineConfig({
  plugins: [
    nxViteTsPaths(),
    {
      ...typescript({
        tsconfig: process.env.NX_TSCONFIG_PATH ?? join(__dirname, 'tsconfig.lib.json'),
        noForceEmit: true,
      }),
      apply: 'build',
    },
  ],
})
```
@pulecka pulecka force-pushed the feat/vite-generated-lib-app-config branch from 34bd768 to 507ff07 Compare December 5, 2023 08:57
@pulecka
Copy link
Contributor Author

pulecka commented Dec 5, 2023

@mandarini @belaczek rebased with latest master.

@mandarini mandarini enabled auto-merge (squash) December 5, 2023 09:04
@mandarini mandarini merged commit e886923 into nrwl:master Dec 5, 2023
6 checks passed
@pulecka pulecka deleted the feat/vite-generated-lib-app-config branch December 8, 2023 16:40
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants