feat(typescript): update to typescript 4.3.5 #3103
Conversation
update typescript to v4.3.5
| * @param tags An array of JSDocTagInfo objects. | ||
| * @return An array of CompilerJsDocTagInfo objects. | ||
| */ | ||
| export const mapJSDocTagInfo = (tags: ts.JSDocTagInfo[]): d.CompilerJsDocTagInfo[] => { |
There was a problem hiding this comment.
Can we mark the tags parameter as readonly?
| export const mapJSDocTagInfo = (tags: ts.JSDocTagInfo[]): d.CompilerJsDocTagInfo[] => { | |
| export const mapJSDocTagInfo = (tags: ReadonlyArray<ts.JSDocTagInfo>): d.CompilerJsDocTagInfo[] => { |
(I don't think we can do the same with the return type, because that would lead to changes in other functions, like transform-utils.ts#mapJSDocTagInfo)
There was a problem hiding this comment.
TIL - opted for the equivalent readonly ts.JSDocTagInfo[].
| * @return An array of CompilerJsDocTagInfo objects. | ||
| */ | ||
| export const mapJSDocTagInfo = (tags: ts.JSDocTagInfo[]): d.CompilerJsDocTagInfo[] => { | ||
| return tags.map((tag) => ({ ...tag, text: tag.text?.map((part) => part.text).join('') })); |
There was a problem hiding this comment.
When we have >1 SymbolDisplayPart in a JSDocTagInfo, we're joining them in this change with .join('') - can we add a comment as to why we need to join them? From my own playing around it seems to be related to how the pieces of the JSDoc are structured internally. For example I add some 'doc' to a boilerplate Stencil component library:
/**
* The middle name
* @argument i love to argue
* @async nsync backstreet boys
*/
@Prop() middle: string;Which internally becomes:
{ text: 'i', kind: 'parameterName' }
{ text: ' ', kind: 'space' }
{ text: 'love to argue', kind: 'text' }
{ text: 'nsync backstreet boys', kind: 'text' }
When we create a JSON readme, this all looks right to me:
{
"name": "middle",
"type": "string",
"mutable": false,
"attr": "middle",
"reflectToAttr": false,
"docs": "The middle name",
"docsTags": [
{
"name": "argument",
"text": "i love to argue"
},
{
"name": "async",
"text": "nsync backstreet boys"
}
],
"values": [
{
"type": "string"
}
],
"optional": false,
"required": false
}
],Is my understanding right there?
There was a problem hiding this comment.
Exactly. I've added a comment describing this.
src/testing/mocks.ts
Outdated
| export function mockStencilSystem(): TestingSystem { | ||
| return createTestingSystem(); | ||
| const sys = createTestingSystem(); |
There was a problem hiding this comment.
We have four callsites for createTestingSystem in Stencil. Does it make sense to move the type guard and the associated usage to the end of createTestingSystem() to get better type safety through the codebase?
+ function isTestingSystem(sys: CompilerSystem): sys is TestingSystem {
+ return 'diskReads' in sys && 'diskWrites' in sys;
+ }
+
- export const createTestingSystem = () => {
+ export const createTestingSystem = (): TestingSystem => {
...
- return Object.defineProperties(sys, {
+ const testingSystem = Object.defineProperties(sys, {
diskReads: {
get() {
return diskReads;
},
set(val: number) {
diskReads = val;
},
},
diskWrites: {
get() {
return diskWrites;
},
set(val: number) {
diskWrites = val;
},
},
});
+ if (!isTestingSystem(testingSystem)) {
+ throw new Error('could not generate TestingSystem');
+ }
+ return testingSystem;This would allow us to refrain from 'knowing we need to put a typeguard here' everytime we call this testing function, and helps us make the guarantee in the function signature of createTestingSystem that this is going to return a TestingSystem
There was a problem hiding this comment.
Good point. It's a little unclear why we have both mockStencilSystem() and createTestingSystem(). I took the liberty to move the TestingSystem interface to testing-sys.ts as well. FWIW, the only place we actually use the diskReads and diskWrites properties is in in-memory-fs.spec.ts
| describe('transform utils', () => { | ||
| it('flattens TypeScript JSDocTagInfo to Stencil JSDocTagInfo', () => { | ||
| const tags = [ | ||
| { |
There was a problem hiding this comment.
Nit: Can we add a test for no/empty text?
| { | |
| { | |
| name: 'param', | |
| text: [], | |
| }, | |
| { |
| ]; | ||
|
|
||
| expect(mapJSDocTagInfo(tags)).toEqual([ | ||
| { name: 'param', text: 'foo the first parameter' }, |
There was a problem hiding this comment.
To coincide with my suggestion above:
| { name: 'param', text: 'foo the first parameter' }, | |
| { name: 'param', text: '' }, | |
| { name: 'param', text: 'foo the first parameter' }, |
There was a problem hiding this comment.
LGTM! Going to hold off on merging, as I think @splitinfinities wanted to give this a look over as well (if that's not the case I'll merge it once he says either way)
|
This upgrade worked great on the Apps & Component Libraries that I compiled. Great job! |
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm test) were run locally and passednpm run test.karma.prod) were run locally and passednpm run prettier) was run locally and passedPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Stencil is compiled using TypeScript 4.2.3.
What is the new behavior?
Stencil is compiled using TypeScript 4.3.5.
Does this introduce a breaking change?
Testing
npm run build,npm test,npm run test.karma.prodOther information