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.
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.
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
| async function waitForDependencyResolution(loadPromise: LoadModulePromise): Promise<void> { | ||
| const [resolveStaticDependencyPromises, resolveDynamicImportPromises] = await loadPromise; | ||
| return Promise.all([...resolveStaticDependencyPromises, ...resolveDynamicImportPromises]); | ||
| await Promise.all([...resolveStaticDependencyPromises, ...resolveDynamicImportPromises]); |
There was a problem hiding this comment.
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.
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.
Again the same question as with the AstContext
There was a problem hiding this comment.
see above, I'll revert.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description