-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(scripts): improve API violation reporting #25356
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
Changes from all commits
f0d759b
7f85801
3faca13
e1376fb
d8e1e82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,6 @@ import * as fs from 'fs'; | |
| import * as path from 'path'; | ||
| import * as jju from 'jju'; | ||
| import type { TscTaskOptions } from 'just-scripts'; | ||
| import { offsetFromRoot } from '@nrwl/devkit'; | ||
| import { appRootPath } from '@nrwl/tao/src/utils/app-root'; | ||
|
|
||
| export function getTsPathAliasesConfig() { | ||
| const cwd = process.cwd(); | ||
|
|
@@ -46,26 +44,34 @@ export function getTsPathAliasesApiExtractorConfig(options: { | |
| tsConfigPath: string; | ||
| packageJson: PackageJson; | ||
| }) { | ||
| const rootOffset = offsetFromRoot(path.dirname(options.tsConfigPath.replace(appRootPath, ''))); | ||
| /** | ||
| * This special TSConfig config is all that's needed for api-extractor so it has all type information used for package: | ||
| * Customized TSConfig that uses `tsconfig.lib.json` as base with some required overrides: | ||
| * | ||
| * NOTES: | ||
| * - `compilerOptions.paths` doesn't work, nor is possible to turn them off when `extends` is used | ||
| * - `extends` is properly resolved via api-extractor which uses TS api | ||
| * - `skipLibCheck` needs to be explicitly set to `false` so errors propagate to api-extractor | ||
| * - `paths` is set to `undefined` so api-extractor won't use source files rather rollup-ed declaration files only | ||
| * | ||
| */ | ||
| const apiExtractorTsConfig: TsConfig = { | ||
| include: options.tsConfig.include, | ||
| /** | ||
| * `files` might be used to specify additional `d.ts` or global type definitions. IF they exist in package tsconfig we need to include them | ||
| */ | ||
| ...(options.tsConfig.files ? { files: options.tsConfig.files } : null), | ||
| ...options.tsConfig, | ||
| compilerOptions: { | ||
| ...options.tsConfig.compilerOptions, | ||
| ...enableAllowSyntheticDefaultImports({ pkgJson: options.packageJson }), | ||
| strict: true, | ||
| lib: options.tsConfig.compilerOptions.lib, | ||
| typeRoots: ['node_modules/@types', `${rootOffset}typings`], | ||
| types: options.tsConfig.compilerOptions.types, | ||
| /** | ||
| * This option has no effect on type declarations '.d.ts' thus can be turned off. For more info see https://www.typescriptlang.org/tsconfig#non-module-files | ||
| * | ||
| * NOTE: Some v8 packages (font-icons-mdl2) use `preserveConstEnums: false` which clashes with isolateModules - TSC will error | ||
| */ | ||
| isolatedModules: false, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in v9 packages it would be nice to set this to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is set to true in all v9 packages already. isn't the documentation on this property clear ? that it has no effect on declaration files ? the v8 context:
I was digging more into the icons-mdl2 issue but changing that to emit cons enums would lead to increased js payload size which we dont know what implications that might bring. ATM the problematic api is marked as Removing the deprecated would be breaking change unfortunately.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, but it would be nice to be consistent with the compiler options we use in our own tsconfigs. I can see the problem with v8 and const enums tho. thx for explaining |
||
| /** | ||
| * needs to be explicitly set to `false` so errors propagate to api-extractor | ||
| */ | ||
| skipLibCheck: false, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this affect libraries that are not in the monorepo ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you elaborate more on your question pls ? what libraries are you referring to. we need strict library checking turned on when generating rolluped type so we give customers 100% guarantee we ship correct API's. if there is some 3rd party library that has some type violations only we will be affected as that will fail the api generation(pipeline). One example of such a violation is storybook as it uses
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh my bad, I misread and thought that we would be type checking 3rd party libraries |
||
| /** | ||
| * just-scripts provides invalid types for tsconfig, thus `paths` cannot be set to dictionary,nor null or `{}` | ||
| */ | ||
| paths: undefined, | ||
| }, | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't options be spread at the end to override the defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. those are "The defaults" from
tsconfig.lib.json. we override thoseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok that makes sense