-
Notifications
You must be signed in to change notification settings - Fork 666
[lockfile-explorer] Isolate .pnpmcfile.cjs execution and add syntax highlighter #5366
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
Conversation
| }); | ||
| } | ||
|
|
||
| public async disposeAsync(): Promise<void> { |
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.
You should use [Symbol.asyncDispose] so that this object can be used with await using
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.
Good suggestion. Fixed.
I had to upgrade Prettier to support this syntax
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.
Turns out that await using is not supported by Node 18 or Jest. I will revert those changes.
| pnpmfileModuleError = error; | ||
| } | ||
|
|
||
| parentPort?.on('message', async (message: IRequestMessage) => { |
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.
parentPort not existing should be an immediate fatal error.
| let pnpmfileModuleError: Error | undefined = undefined; | ||
|
|
||
| try { | ||
| if (fs.existsSync(resolvedPath)) { |
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.
This is redundant. You're already catching the error, and you should skip the worker altogether if you know the file doesn't exist to save on all the messaging overhead.
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.
Fixed
| } | ||
|
|
||
| try { | ||
| if (!pnpmfileModule || !pnpmfileModule.hooks || typeof pnpmfileModule.hooks.readPackage !== 'function') { |
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.
I'd suggest instead sending a message on initialization to the host telling it which hooks are present, then don't bother sending messages when the readPackage hook isn't defined.
Combine with an initializeAsync method on PnpmfileRunner that you wait for before calling the individual APIs.
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.
It's a good idea, but in the interest of time I need to keep the implementation simple for now.
| } | ||
|
|
||
| parentPort?.on('message', async (message: IRequestMessage) => { | ||
| const { id, packageJson } = message; |
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.
I find it generally useful to have false be one of the valid message values, and have that be a signal to gracefully shut down the worker. See, e.g. the ModuleMinifierWorker.
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.
This is a good suggestion but I would need to add some safeguards to guarantee the child really does terminate. I'll do it in a future PR.
| const packageJSONFile: IPackageJson | undefined = await readPackageJsonAsync(packageName); | ||
| setPackageJSON(packageJSONFile); | ||
| const parsedJSON = await readPackageSpecAsync(packageName); | ||
| const parsedJSON: IPackageJson | undefined = await readPackageSpecAsync(packageName); | ||
| setParsedPackageJSON(parsedJSON); |
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.
Parallelize these with Promise.all?
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 premature optimization at this point.
apps/lockfile-explorer-web/src/containers/PackageJsonViewer/CodeBox.tsx
Outdated
Show resolved
Hide resolved
| export type PrismLanguage = | ||
| | 'plain' | ||
| | 'plaintext' | ||
| | 'text' | ||
| | 'txt' | ||
| | 'markup' | ||
| | 'html' | ||
| | 'mathml' | ||
| | 'svg' | ||
| | 'xml' | ||
| | 'ssml' | ||
| | 'atom' | ||
| | 'rss' | ||
| | 'regex' | ||
| | 'clike' | ||
| | 'javascript' | ||
| | 'js' | ||
| | 'actionscript' | ||
| | 'coffeescript' | ||
| | 'coffee' | ||
| | 'javadoclike' | ||
| | 'css' | ||
| | 'yaml' | ||
| | 'yml' | ||
| | 'markdown' | ||
| | 'md' | ||
| | 'graphql' | ||
| | 'sql' | ||
| | 'typescript' | ||
| | 'ts' | ||
| | 'jsdoc' | ||
| | 'flow' | ||
| | 'n4js' | ||
| | 'n4jsd' | ||
| | 'jsx' | ||
| | 'tsx' | ||
| | 'swift' | ||
| | 'kotlin' | ||
| | 'kt' | ||
| | 'kts' | ||
| | 'c' | ||
| | 'objectivec' | ||
| | 'objc' | ||
| | 'reason' | ||
| | 'rust' | ||
| | 'go' | ||
| | 'cpp' | ||
| | 'python' | ||
| | 'py' | ||
| | 'json' | ||
| | 'webmanifest'; |
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.
| export type PrismLanguage = | |
| | 'plain' | |
| | 'plaintext' | |
| | 'text' | |
| | 'txt' | |
| | 'markup' | |
| | 'html' | |
| | 'mathml' | |
| | 'svg' | |
| | 'xml' | |
| | 'ssml' | |
| | 'atom' | |
| | 'rss' | |
| | 'regex' | |
| | 'clike' | |
| | 'javascript' | |
| | 'js' | |
| | 'actionscript' | |
| | 'coffeescript' | |
| | 'coffee' | |
| | 'javadoclike' | |
| | 'css' | |
| | 'yaml' | |
| | 'yml' | |
| | 'markdown' | |
| | 'md' | |
| | 'graphql' | |
| | 'sql' | |
| | 'typescript' | |
| | 'ts' | |
| | 'jsdoc' | |
| | 'flow' | |
| | 'n4js' | |
| | 'n4jsd' | |
| | 'jsx' | |
| | 'tsx' | |
| | 'swift' | |
| | 'kotlin' | |
| | 'kt' | |
| | 'kts' | |
| | 'c' | |
| | 'objectivec' | |
| | 'objc' | |
| | 'reason' | |
| | 'rust' | |
| | 'go' | |
| | 'cpp' | |
| | 'python' | |
| | 'py' | |
| | 'json' | |
| | 'webmanifest'; | |
| export type PrismLanguage = Omit<import('prism-react-renderer').Prism.languages, "extend" | "insertBefore" | "DFS"> |
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.
These strings are not expressed in the Prism .d.ts file. They are apparently only available at runtime.
| // "The following entrypoint(s) combined asset size exceeds the recommended limit." | ||
| // maxEntrypointSize: 500000, | ||
| // maxAssetSize: 500000 | ||
| maxEntrypointSize: Infinity, |
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.
If you're setting these to infinity, why not just turn off performance?
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.
@iclanton Is there a better way to do that? I tried undefined but in the merge it doesn't undo the rig's setting.
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.
It's performance: false
| const pnpmfilePath: string = path.join( | ||
| appState.lfxWorkspace.workspaceRootFullPath, | ||
| appState.lfxWorkspace.pnpmLockfileFolder, | ||
| '.pnpmfile.cjs' | ||
| ); |
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.
| const pnpmfilePath: string = path.join( | |
| appState.lfxWorkspace.workspaceRootFullPath, | |
| appState.lfxWorkspace.pnpmLockfileFolder, | |
| '.pnpmfile.cjs' | |
| ); | |
| const { lfxWorkspace: { workspaceRootFullPath, pnpmLockfileFolder } } = appState; | |
| const pnpmfilePath: string = `${workspaceRootFullPath}/${pnpmLockfileFolder}/.pnpmfile.cjs`; |
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.
This would incorrectly create double-slashes when pnpmLockfileFolder is the empty string
| // debugger; | ||
|
|
||
| const { pnpmfilePath } = workerData; | ||
| const resolvedPath: string = path.resolve(pnpmfilePath); |
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.
Is this ever not absolute?
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.
It is absolute, but only because in the current implementation, an absolute path is used by the caller of the class constructor in the other thread. It is a very brittle assumption.
…the generated temp file
Summary
More progress updating Lockfile Explorer.
Details
-
.pnpmcfile.cjsis now evaluated in an isolated thread; the earlier approach of simply callingrequire()on it would cause memory leaks and incorrect cachingHow it was tested
Impacted documentation
None.
@iclanton @nickpape @william2958 @dmichon-msft