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

Don't error on missing re-exported declarations #332

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jackw
Copy link

@jackw jackw commented Sep 5, 2024

Hey 👋

Many thanks for creating this package. It's really useful! 👏👏👏

I've come across a situation where some packages re-export types but due to having the re-exported types dependency set as a dev dependency it isn't installed via npm and dts-bundle-generator errors out.

// node_modules/my-package/dist/index.d.ts
export { TypeA, TypeB } from '@types/some-package'
Error while bundling types: TypeError: Cannot read properties of undefined (reading 'declarations')
    at getDeclarationsForSymbol (/Users/jackwestbrook/dev/grafana/plugin-tools/node_modules/dts-bundle-generator/dist/helpers/typescript.js:114:16)
    at TypesUsageEvaluator.computeUsageForNode (/Users/jackwestbrook/dev/grafana/plugin-tools/node_modules/dts-bundle-generator/dist/types-usage-evaluator.js:117:97)
    at visitNodes (/Users/jackwestbrook/dev/grafana/plugin-tools/node_modules/typescript/lib/typescript.js:31751:22)
    at forEachChildInSourceFile (/Users/jackwestbrook/dev/grafana/plugin-tools/node_modules/typescript/lib/typescript.js:31964:12)
    at Object.forEachChild (/Users/jackwestbrook/dev/grafana/plugin-tools/node_modules/typescript/lib/typescript.js:32264:35)
    at TypesUsageEvaluator.computeUsages (/Users/jackwestbrook/dev/grafana/plugin-tools/node_modules/dts-bundle-generator/dist/types-usage-evaluator.js:59:16)
    at new TypesUsageEvaluator (/Users/jackwestbrook/dev/grafana/plugin-tools/node_modules/dts-bundle-generator/dist/types-usage-evaluator.js:12:14)
    at generateDtsBundle (/Users/jackwestbrook/dev/grafana/plugin-tools/node_modules/dts-bundle-generator/dist/bundle-generator.js:20:33)
    at generateTypes (file:///Users/jackwestbrook/dev/grafana/plugin-tools/packages/plugin-types-bundler/dist/bundleTypes.js:22:17)
    at file:///Users/jackwestbrook/dev/grafana/plugin-tools/packages/plugin-types-bundler/dist/bin/run.js:18:5

This error was quite confusing for me and solving it consisted of installing the missing package in my project. However I think it might make sense to offer the option to warn users that it's missing and allow execution to continue. Alternatively maybe it's enough to throw a more helpful error msg and force the user to install the missing package to make sure types are correctly bundled. I'm opening this PR as a starting point to discuss this further.

From my investigations without the re-exported package installed it seems typescript treats the missing re-exported packages as any and doesn't seem to surface any errors (at least none that I could find).

Additionally whilst doing this I noticed there is no way to change the log level when using this packages API. Is that something you'd consider adding as an option?

@jackw jackw changed the title Don't bail on missing re-exported declarations Don't error on missing re-exported declarations Sep 6, 2024
@timocov
Copy link
Owner

timocov commented Sep 15, 2024

However I think it might make sense to offer the option to warn users that it's missing and allow execution to continue

I'm not sure I agree with this.

This error was quite confusing for me and solving it consisted of installing the missing package in my project

This feels like the only correct solution in a given situation, and the reason for this is because if a package is marked as "dev dependency" how come you're able to import it (even via transitive files)? It feels like some sort of misconfiguration. Can you share an example what packages generate such situation?

From my investigations without the re-exported package installed it seems typescript treats the missing re-exported packages as any and doesn't seem to surface any errors (at least none that I could find).

Do you have skipLibCheck option on by any chance?

Additionally whilst doing this I noticed there is no way to change the log level when using this packages API. Is that something you'd consider adding as an option?

Never thought about it tbh, I'm open for suggestions tho.

@jackw
Copy link
Author

jackw commented Sep 26, 2024

Hey @timocov,

thanks so much for getting back to me on this and for giving me some valuable insights into what's going on.

I'm not sure I agree with this.

That's fair enough and I completely understand why you'd want to fail if declarations are missing.

This feels like the only correct solution in a given situation, and the reason for this is because if a package is marked as "dev dependency" how come you're able to import it (even via transitive files)? It feels like some sort of misconfiguration. Can you share an example what packages generate such situation?

Do you have skipLibCheck option on by any chance?

Looking at our shared tsconfig package, yes we do have it enabled across our org. 😞

Based on your questions and a little digging this is what I've understood now...

  1. @grafana/[email protected] re-exports types from @types/react-table in its declaration file - export { CellProps, SortByFn } from 'react-table';.
  2. @types/react-table is a devDependency so isn't installed when a consuming project installs @grafana/ui.
  3. We also install @grafana/tsconfig in the same project and have skipLibCheck: true that prevents surfacing the missing types errors within the project.
  4. Only when it comes to rolling up a project that consumes types from @grafana/ui using dts-bundle-generator does the error occur because the bundler cannot find the declarations. Meanwhile typescript itself is fine with it because of skipLibCheck.

Disabling skipLibCheck seems to be somewhat tricky for us due to this issue with @types/react-table.

This is clearly a misconfiguration on our part and we need to figure out how to solve that at our end properly. For now it's been fixes in the 11.2.0 release of grafana/ui by moving the re-exported @types/* packages from devDependencies to dependencies.

Even so, would you be willing to accept a PR that would catch the error and re-throw it along with a helpful message in this scenario that a declaration for "package x" cannot be found?

Never thought about it tbh, I'm open for suggestions tho.

I'm using the package programatically along with another TS related package of ours to inline all imported packages when generating a types file. Being able to set the log level at the API to surface issues/errors would be quite helpful.

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.

2 participants