chore: create new tsconfig.base with path aliases #16976
Conversation
| "skipLibCheck": true, | ||
| "typeRoots": ["../../node_modules/@types", "../../typings"], | ||
| "types": ["jest", "webpack-env", "custom-global"] | ||
| "types": ["jest", "custom-global"] |
There was a problem hiding this comment.
cleanup: webpack-env is already included in custom-globals
| * > - 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.
workaround to make existing pipeline work without changes
| @@ -0,0 +1,24 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "target": "ES2015", | |||
There was a problem hiding this comment.
we gonna use this also for node packages etc, particular packages should explicitly state their target (in future babel/esbuild will handle that automatically)
| "compilerOptions": { | ||
| "baseUrl": ".", | ||
| "target": "ES5", |
There was a problem hiding this comment.
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
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Hotell
changed the title
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5ff03c4:
|
Asset size changesUnable to find bundle size details for Baseline commit: 9786d01 Possible causes
Recommendations
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Hotell
force-pushed
the
hotell/16889/ts-aliases
branch
from
11926b3
to
522bf77
Compare
Hotell
requested review from
dzearing,
ecraig12345,
layershifter,
ling1726 and
xugao
as code owners
| "@fluentui/react-menu": ["packages/react-menu/src/index.ts"] | ||
| } | ||
| }, | ||
| "exclude": ["node_modules"] |
There was a problem hiding this comment.
Just curious, why is it necessary to explicitly do this?
There was a problem hiding this comment.
do what? provide exclude ?
- it's not necessary, but for consistency and smaller git diffs I added it so it can be easily extended by other folders/globs (which we may introduce)
There was a problem hiding this comment.
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.
| "importHelpers": true, | ||
| "noUnusedLocals": true, |
There was a problem hiding this comment.
These should probably be in the defaults too, or at least noUnusedLocals should (it's not included by default with strict)
There was a problem hiding this comment.
from my experience noUnusedLocals is anti pattern/distracting churn to devs
- to be consistent this should be handled by eslint (TS is used for type checking for linting)
- unused code will be removed by minifier
- feedback from most of devs I worked in my entire carer was quite consistent: getting your app builds fail during dev just because you have some unused stuff is extremely distracting.
There was a problem hiding this comment.
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 noUnusedLocals in the default config.
There was a problem hiding this comment.
If we're going to make that change we should probably have an issue to track it.
I was thinking this should be rather an RFC from point of DX change , than an issue.
tsconfig.base.json
Outdated
| "forceConsistentCasingInFileNames": true, | ||
| "esModuleInterop": true, |
There was a problem hiding this comment.
I don't think this should be included in the defaults since it can cause problems downstream if not all consumers are also using it. IIRC @layershifter may have more background here.
There was a problem hiding this comment.
As I remember it will require esModuleInterop: true on customer's side, too. Not all our customers have it enabled.
I failed to find a PR that have been done a long time ago, but at least a commit is there (microsoft/fluent-ui-react@75c139d).
There was a problem hiding this comment.
I failed to find a PR that have been done a long time ago, but at least a commit is there (microsoft/fluent-ui-react@75c139d).
unfortunately that's not really helpful, no reasoning etc :)
Overall I'm not aware of any implications to customers nor about a requirement to have this enabled as well in consumers setup.
esModuleInterop helps to consume non es2015 module code with JS module syntax and normalizes ambient definitions consumption. That's it (no changes to export declaration emit for consumers, nor from runtime perspective). Unless you can provide explicit issue I'm gonna keep it as is. thx
There was a problem hiding this comment.
I created a sample project, https://github.com/layershifter/tsc-test
Where:
hostis an application that hasesModuleInterop: trueand builds type definitionsconsumeris an application that hasesModuleInterop: false(i.e. default value) and consumeshost
To run it:
- clone
yarnyarn build
You will a build failure:
$ tsc
../host/dist/index.d.ts:1:8 - error TS1259: Module '"C:/Users/olfedias/WebstormProjects/tsc-test/node_modules/@types/react/index"' can only be default-imported using the 'esModuleInterop' flag
1 import React from 'react';
~~~~~
../../node_modules/@types/react/index.d.ts:69:1
69 export = React;
~~~~~~~~~~~~~~~
This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.
skipLibCheck: true can workaround it, but it also defaults to false. If we will enable esModuleInterop, produced artifacts cannot be consumed by customers with default TypeScript configs.
There was a problem hiding this comment.
Thanks for digging into it. I forgot about this completely (I guess sideffect for using skip lib check for couple of years everywhere)
I run into that in our codebase quite recently as well 🗡️ 74f5de4 )
This leads us back to fundamental question: What are requirements for fluent convergence regarding ambient declarations output ?
All in all this feels like right change not just to us but for every customer.
Why?
- skipLibCheck is recommended to be set to
truein official docs (it's not true by default to not introduce breaking changes, which are introduced anyway even in feature bumps) - if customers enable this setting they will get type checking speed boost "for free"
There was a problem hiding this comment.
IMO, our packages should work with any TS config including default options as consumers can have different motivation for their compilerOptions. For example, esModuleInterop is not enabled in Teams, this change will require them either to enable skipLibCheck everywhere (they might not want to do this everywhere) or esModuleInterop.
Personally I also don't see any value from enabling esModuleInterop (for components' packages) as in general we should not use CommonJS dependencies (they are impacting tree-shaking). We have React's packages as CommonJS dependencies, but they are going to have ESM builds (facebook/react#11503) and star imports (import * as React) are recommended currently anyway:
The default imports will keep working in React 17, but in the longer term we encourage moving away from them.
https://ru.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html#removing-unused-react-imports
There was a problem hiding this comment.
IMO, our packages should work with any TS config
This is way too ambitious statement :) do we support moduleResolution: "classic" etc ? ...
Consumer of a package shouldn't care nor be limited by library internal tsconfig used.
require them either to enable skipLibCheck everywhere
Thats good for them (see my previous comment), TS exec speed benefits.
as in general we should not use CommonJS dependencies
I agree, although esModuleInterop won't help you with that much :) we'd need special lint rule or even better explicit deps validator
(import * as React) are recommended currently anyway:
It's not recommended way rather than "valid way that will not get replaced when running codemod". I already touched this point that we should rather use explicit named imports. anyways this is out off topic in terms of this PR.
There was a problem hiding this comment.
Based on discussion I removed esModuleInterop for this PR ->
as an action point:
can I ask you to create requirements doc for how we want author ambient type declaration @layershifter ? thx
| "experimentalDecorators": true, | ||
| "importHelpers": true, | ||
| "noUnusedLocals": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "strict": true, | ||
| "moduleResolution": "node", | ||
| "preserveConstEnums": true, |
There was a problem hiding this comment.
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.
enums is another anti pattern and has serious implications in type safety. I'd like to avoid it completely in any convergence package. I understand it's still used (even in convergence) but as I already mentioned, base config is gonna be used everywhere, thus this makes no sense for js/tools.
see tip no 4 https://medium.com/@martin_hotell/10-typescript-pro-tips-patterns-with-or-without-react-5799488d6680
There was a problem hiding this comment.
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. preserveConstEnums is probably less important for tooling packages but not significantly harmful either.
There was a problem hiding this comment.
I'll keep this config scoped per package for reasons I mentioned for now.
that should be done with a lint rule
cant agree more. we can/should iterate on linting approach etc in a follow up PR/RFC
|
Meant to say: this is looking good in general, just lots of little comments about specific options. |
Hotell
force-pushed
the
hotell/16889/ts-aliases
branch
from
34384dd
to
1c7c16c
Compare
Hotell
force-pushed
the
hotell/16889/ts-aliases
branch
from
1c7c16c
to
5ff03c4
Compare
* chore: create new tsconfig base with path aliases * chore(scripts): make current tsc just task to work with ts aliases * chore(react-menu): switch to TS path aliases config
Pull request checklist
$ yarn changeDescription of changes
Focus areas to test
(optional)