-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Dynamic import (lazy load) parsers #2231
Conversation
Oooh this is pretty cool and looks clean. I would still want user configurable sync parsers because while lazy loading is good, it is not always optimal. Lazy loading requires more chunks=more separate network requests, more latency before parsing can be started. Your current implementation could support that pretty well I think, in a future it could look something similar to // AiffLoader.ts
export const aiffLazyParserLoader: IParserLoader = {
parserType: 'aiff',
extensions: ['.aif', 'aiff', 'aifc'],
async load(): Promise<ITokenParser> {
return new (await import('./AiffParser.js')).AIFFParser;
}
}; // ApexLoaderSync.ts
import { APEv2Parser } from './APEv2Parser.js'
export const apeParserLoader: IParserLoader = {
parserType: 'apev2',
extensions: ['.ape'],
load(): ITokenParser{
return new APEv2Parser;
}
}; Putting it all together into pseudo code: import { parserBlob } from 'music-metadata';
import { apeParserLoader } from 'music-metadata/loaders-sync';
import { aiffLazyParserLoader } from 'music-metadata/loaders';
parserBlob(blob, { parsers: [apeParserLoader, aiffLazyParserLoader] }) This would make aiff load when needed and ape sync. |
I started thinking more, about my previous comment and came with following points:
So my new proposal would be, to allow passing a function which loads parser when discovered: import { parserBlob } from 'music-metadata/without-parsers'; // to not break old users
import { apeParser } from 'music-metadata/parser/ape';
parserBlob(blob, {
parser: (parserType) => {
if (parserType === 'aiff') {
return import('music-metadata/parser/aiff'')
}
if (parserType === 'ape') {
return apeParser
}
}
}) This allows best of both worlds, now user is in full control when to load parser sync and when async, they can even preload it if they need it. And the API design is simpler. For users who do not care they can import all of the parsers. import { parserBlob } from 'music-metadata/without-parsers'; // to not break old users
import { defaultParsers } from 'music-metadata/parser';
parserBlob(blob, {
parser: defaultParsers
}) |
Glad you like it, still work to do. Some cleanup to to, and I think parser
I see you concerns with lazy loading, and need to put that in a bot of perspective. "Premature optimization is the root of all evil (or at least most of it) in programming"1 Nothing changed since 1974, remember that one. Because the the parsers are lazy loaded, the entire API which is using The other difference is, let's we a user parses a single file, music-metadata is loaded, and only that specific parser. And no more music-metadata plus all the other parser. Especially if these modules are small, and consistently lazy (dynamically) loaded, the user will not even notice it. The user experience is that everything is instantly available. Yes you can try to over-complicate things, and try to eliminate the 10ms load time. But think that is the job of these fancy module bundlers, to whom we clearly defined what is dependent on what, and when what is to be loaded. Let those things optimize, they are designed for that reason. Footnotes
|
The primary use case for |
8c8ac12
to
111eb54
Compare
111eb54
to
7cee540
Compare
Part of v10.4.0 |
@Borewit The latest release reduced chunk using musicmetada from ~180kb to ~104kb in my application. Thank you! |
Awesome!, and interesting as the strtok3 dependencies in |
Use dynamic imports enabling to load parsers in lazy fashion