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
fix: more types #4361
fix: more types #4361
Conversation
517cfb8
to
5cb4167
Compare
Codecov Report
@@ Coverage Diff @@
## master #4361 +/- ##
==========================================
+ Coverage 98.68% 98.70% +0.01%
==========================================
Files 205 205
Lines 7323 7320 -3
Branches 2083 2083
==========================================
- Hits 7227 7225 -2
+ Misses 36 35 -1
Partials 60 60
Continue to review full report at Codecov.
|
9489802
to
b44b46b
Compare
src/Module.ts
Outdated
node: ExportAllDeclaration | ExportNamedDeclaration | ExportDefaultDeclaration | ||
): void => this.addExport(node), | ||
addImport: (node: ImportDeclaration): void => this.addImport(node), | ||
addImportMeta: (node: MetaProperty): void => this.addImportMeta(node), |
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.
What is the advantage of doing this? Considering performance, my understanding is that .bind
is now more efficient as it avoids the indirection of the additional function, and these functions are "hot" in that they are called very often during the tree-shaking phase. And TypeScript should be able to handle .bind
correctly.
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.
yeah, that has nothing to do with typescript. there were perf issues with bind
(in the past), I think those have been fixed tho.
it's just consistency, e.g.: https://github.com/rollup/rollup/pull/4361/files/437e90bc8e8e784d8b3667280d4890dbcfff9945#diff-bdf37a1325be24e5865c89e911c2f1125a1d857ec429afb790bde572bbe12b0fR761-R765
but I'll revert.
src/ModuleLoader.ts
Outdated
const [resolveStaticDependencyPromises, resolveDynamicImportPromises] = await loadPromise; | ||
return Promise.all([...resolveStaticDependencyPromises, ...resolveDynamicImportPromises]); | ||
await Promise.all([...resolveStaticDependencyPromises, ...resolveDynamicImportPromises]); |
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.
To avoid the unnecessary "await as last statement", one could also just type this as Promise<unknown>
. My understanding is that this avoids one implicit layer of Promise unwrapping the compiler has to do.
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.
probably so (albeit probably microscopic in the grand schema of things). I believe I saw this pattern across the code base. I guess there is no "good" way to return undefined
if the caller ignores the function result entirely. I think it makes the code easier to read and also interpret the intend. if the only caller of a function which returns a result ignores the result it feels more like an oversight than done on purpose. that's all.
I'll revert.
src/utils/PluginDriver.ts
Outdated
this.getFileName = this.fileEmitter.getFileName.bind(this.fileEmitter); | ||
this.finaliseAssets = this.fileEmitter.assertAssetsFinalized.bind(this.fileEmitter); | ||
this.setOutputBundle = this.fileEmitter.setOutputBundle.bind(this.fileEmitter); | ||
this.emitFile = (emittedFile: EmittedFile) => this.fileEmitter.emitFile(emittedFile); |
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.
Again the same question as with the AstContext
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.
see above, I'll revert.
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.
Great improvements 👍
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description