-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
chore: create new tsconfig.base with path aliases #16976
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "none", | ||
"comment": "chore(react-menu): switch to TS path aliases config", | ||
"packageName": "@fluentui/react-menu", | ||
"email": "[email protected]", | ||
"dependentChangeType": "none" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,16 @@ | ||
{ | ||
"extends": "../../tsconfig.base.json", | ||
"compilerOptions": { | ||
"baseUrl": ".", | ||
"target": "ES5", | ||
"lib": ["ES5", "dom"], | ||
"outDir": "dist", | ||
"target": "es5", | ||
"module": "commonjs", | ||
"jsx": "react", | ||
"declaration": true, | ||
Hotell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"sourceMap": true, | ||
"experimentalDecorators": true, | ||
Hotell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"importHelpers": true, | ||
"noUnusedLocals": true, | ||
Comment on lines
10
to
11
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. These should probably be in the defaults too, or at least 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. from my experience
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. Hmm switching to enforcement with lint instead of TS seems reasonable. If we're going to make that change we should probably have an issue to track it. Though until that's implemented it might still be worth having 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.
I was thinking this should be rather an RFC from point of DX change , than an issue. |
||
"forceConsistentCasingInFileNames": true, | ||
"strict": true, | ||
"moduleResolution": "node", | ||
"preserveConstEnums": true, | ||
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. This should also be in the defaults. Not preserving const enums can cause problems for consumers in some cases. 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.
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. Possibly a valid point, but it's not something we've discussed specifically in this repo (aside from that we must preserve const enums due to compat issues with other build systems). If we want to ban enums, that should be done with a lint rule. Until then, we should have defaults that reasonably handle the last agreed upon state of things, which currently is that enums are allowed. 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. I'll keep this config scoped per package for reasons I mentioned for now.
|
||
"lib": ["es5", "dom"], | ||
"skipLibCheck": true, | ||
"typeRoots": ["../../node_modules/@types", "../../typings"], | ||
"types": ["jest", "webpack-env", "custom-global"] | ||
"types": ["jest", "custom-global"] | ||
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. cleanup: |
||
}, | ||
"include": ["src"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,48 +1,97 @@ | ||
import fs from 'fs'; | ||
import * as path from 'path'; | ||
import { tscTask, argv } from 'just-scripts'; | ||
import { tscTask, argv, TscTaskOptions, resolveCwd, logger } from 'just-scripts'; | ||
import { Arguments } from 'yargs'; | ||
import jju from 'jju'; | ||
|
||
interface JustArgs extends Arguments { | ||
production?: boolean; | ||
} | ||
|
||
interface TsConfig { | ||
extends?: string; | ||
compilerOptions: import('typescript').CompilerOptions; | ||
include?: string[]; | ||
exclude?: string[]; | ||
} | ||
|
||
const libPath = path.resolve(process.cwd(), 'lib'); | ||
const srcPath = path.resolve(process.cwd(), 'src'); | ||
// Temporary hack: only use tsbuildinfo file for things under packages/fluentui | ||
const useTsBuildInfo = | ||
/[\\/]packages[\\/]fluentui[\\/]/.test(process.cwd()) && path.basename(process.cwd()) !== 'perf-test'; | ||
|
||
function getExtraTscParams(args) { | ||
/** | ||
* | ||
Hotell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* Explicitly set `baseUrl` to current package root for packages (converged packages) that use TS path aliases. | ||
* > - This is a temporary workaround for current way of building packages. | ||
* > - Without setting baseUrl we would get all aliased packages build within outDir | ||
*/ | ||
function backportTsAliasedPackages(options: TscTaskOptions) { | ||
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. workaround to make existing pipeline work without changes |
||
const tsConfigFilePath = resolveCwd('./tsconfig.json'); | ||
const tsConfig: TsConfig = jju.parse(fs.readFileSync(tsConfigFilePath, 'utf-8')); | ||
|
||
const normalizedOptions = { ...options }; | ||
|
||
if (tsConfig.extends) { | ||
logger.info(`package is using TS path aliases. Overriding baseUrl to package root.`); | ||
normalizedOptions.baseUrl = '.'; | ||
} | ||
|
||
return normalizedOptions; | ||
} | ||
|
||
function getExtraTscParams(args: JustArgs) { | ||
return { | ||
pretty: true, | ||
target: 'es5', | ||
// sourceMap must be true for inlineSources and sourceRoot to work | ||
...(args.production && { inlineSources: true, sourceRoot: path.relative(libPath, srcPath), sourceMap: true }), | ||
}; | ||
} | ||
|
||
function getJustArgv(): JustArgs { | ||
return argv(); | ||
} | ||
|
||
export const ts = { | ||
commonjs: () => { | ||
const extraOptions = getExtraTscParams(argv()); | ||
return tscTask({ | ||
const extraOptions = getExtraTscParams(getJustArgv()); | ||
const options = backportTsAliasedPackages({ | ||
...extraOptions, | ||
outDir: 'lib-commonjs', | ||
module: 'commonjs', | ||
...(useTsBuildInfo && { tsBuildInfoFile: '.commonjs.tsbuildinfo' }), | ||
}); | ||
|
||
return tscTask(options); | ||
}, | ||
esm: () => { | ||
const extraOptions = getExtraTscParams(argv()); | ||
const extraOptions = getExtraTscParams(getJustArgv()); | ||
const options = backportTsAliasedPackages({ | ||
...extraOptions, | ||
outDir: 'lib', | ||
module: 'esnext', | ||
}); | ||
|
||
// Use default tsbuildinfo for this variant | ||
return tscTask({ ...extraOptions, outDir: 'lib', module: 'esnext' }); | ||
return tscTask(options); | ||
}, | ||
amd: () => { | ||
const extraOptions = getExtraTscParams(argv()); | ||
return tscTask({ | ||
const extraOptions = getExtraTscParams(getJustArgv()); | ||
const options = backportTsAliasedPackages({ | ||
...extraOptions, | ||
outDir: 'lib-amd', | ||
module: 'amd', | ||
...(useTsBuildInfo && { tsBuildInfoFile: '.amd.tsbuildinfo' }), | ||
}); | ||
|
||
return tscTask(options); | ||
}, | ||
commonjsOnly: () => { | ||
const extraOptions = getExtraTscParams(argv()); | ||
const extraOptions = getExtraTscParams(getJustArgv()); | ||
// Use default tsbuildinfo for this variant (since it's the only variant) | ||
return tscTask({ ...extraOptions, outDir: 'lib', module: 'commonjs' }); | ||
const options = backportTsAliasedPackages({ ...extraOptions, outDir: 'lib', module: 'commonjs' }); | ||
|
||
return tscTask(options); | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
{ | ||
"compilerOptions": { | ||
"target": "ES2015", | ||
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. we gonna use this also for node packages etc, particular packages should explicitly state their target (in future babel/esbuild will handle that automatically) |
||
"module": "esnext", | ||
"moduleResolution": "node", | ||
"lib": ["ES2015", "dom"], | ||
"sourceMap": true, | ||
"strict": true, | ||
"strictFunctionTypes": false, | ||
"forceConsistentCasingInFileNames": true, | ||
"skipLibCheck": true, | ||
"typeRoots": ["node_modules/@types", "./typings"], | ||
"baseUrl": ".", | ||
"paths": { | ||
"@fluentui/set-version": ["packages/set-version/src/index.ts"], | ||
"@fluentui/react-conformance": ["packages/react-conformance/src/index.ts"], | ||
"@fluentui/react-utilities": ["packages/react-utilities/src/index.ts"], | ||
"@fluentui/react-make-styles": ["packages/react-make-styles/src/index.ts"], | ||
"@fluentui/keyboard-key": ["packages/keyboard-key/src/index.ts"], | ||
"@fluentui/react-menu": ["packages/react-menu/src/index.ts"] | ||
} | ||
}, | ||
"exclude": ["node_modules"] | ||
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. Just curious, why is it necessary to explicitly do this? 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. do what? provide
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. Yes the comment was supposed to be specifically on the exclude line. I was just surprised to see this since our configs haven't needed to explicitly exclude node_modules in the past. |
||
} |
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.
TODO (create issue Martin): while we restrict to ES5 ECMA language features, because of global leaking types, we can use almost all ECMA lang features-> it starts with conformance lib , which imports enzyme -> enzyme refs whole node types -> node types add ES2017 etc to the type check tree
#17101
This comment was marked as off-topic.
Sorry, something went wrong.