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

refactor(module-utils): 24.20x speed-up for generate dts tests #2315

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

barak007
Copy link
Collaborator

@barak007 barak007 commented Feb 7, 2022

use noLib:true since we don't use anything from the outside world in the generate d.ts tests

@AviVahl
Copy link
Collaborator

AviVahl commented Feb 7, 2022

May want to validate there are no diagnostics when dom lib is included. Can use skipDefaultLibCheck instead to skip rechecking dom types themselves.

@barak007
Copy link
Collaborator Author

barak007 commented Feb 7, 2022

The majority of the tests does not include anything from output world so they don't need any libs. We have one test with dom libs enabled but now it's specified in the test.

@barak007
Copy link
Collaborator Author

barak007 commented Feb 7, 2022

These tests used to take ~30 sec in CI.... now they take at most 1-2 sec

@barak007 barak007 changed the title refactor(module-utils): 10x speed-up for generate dts tests refactor(module-utils): 30x speed-up for generate dts tests Feb 7, 2022
@AviVahl
Copy link
Collaborator

AviVahl commented Feb 7, 2022

Why did the diagnostics change if the types aren't used? I think the Record type is not loaded or something like that. Is the test verifying there are no new diagnostics?

@barak007
Copy link
Collaborator Author

barak007 commented Feb 7, 2022

Why did the diagnostics change if the types aren't used? I think the Record type is not loaded or something like that. Is the test verifying there are no new diagnostics?

No...I will check.
The change is because the infer of the record {state1: true} changed from true to boolean I'm not sure why and I'm not sure that it matters as I don't expect {state: 'str'} to inferred as str but as string

@barak007
Copy link
Collaborator Author

barak007 commented Feb 7, 2022

No new errors....most of the tests are also expecting '' as the output

@AviVahl
Copy link
Collaborator

AviVahl commented Feb 7, 2022

The types use the Record type: https://github.com/wix/stylable/blob/master/packages/runtime/src/types.ts#L26
I'd really expect that to show up as another diagnostic, and the test to catch that.
Instead of the noLib, use noDefaultLibCheck and specify the types always. wouldn't that be fast enough?

@barak007
Copy link
Collaborator Author

barak007 commented Feb 7, 2022

noDefaultLibCheck helps a little. it cuts it in half.

@barak007
Copy link
Collaborator Author

barak007 commented Feb 7, 2022

The types use the Record type: https://github.com/wix/stylable/blob/master/packages/runtime/src/types.ts#L26 I'd really expect that to show up as another diagnostic, and the test to catch that. Instead of the noLib, use noDefaultLibCheck and specify the types always. wouldn't that be fast enough?

it does if you use it directly. that's interesting will do more checks

@barak007 barak007 marked this pull request as draft February 7, 2022 01:29
@barak007
Copy link
Collaborator Author

barak007 commented Feb 7, 2022

I will go on the safe side and bring back the lib.es2020.d.ts with the noDefaultLibCheck it still 20x faster!. I had the wrong measurements. 10x @AviVahl

@barak007 barak007 marked this pull request as ready for review February 7, 2022 01:37
@barak007 barak007 changed the title refactor(module-utils): 30x speed-up for generate dts tests refactor(module-utils): 24.20x speed-up for generate dts tests Feb 7, 2022
@AviVahl
Copy link
Collaborator

AviVahl commented Feb 7, 2022

just noticed that NodeRenderer also uses Element.
all .d.ts tests that use the stylesheet runtime types are supposed to have dom then.

@barak007 barak007 requested a review from AviVahl February 7, 2022 10:14
@barak007
Copy link
Collaborator Author

barak007 commented Feb 7, 2022

the d.ts does not use these types. actually nothing uses these types from the renderer.

@barak007 barak007 merged commit ae7ff21 into master Feb 7, 2022
@barak007 barak007 deleted the barak/10x-speedup-for-dts-tests branch February 7, 2022 13:01
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