-
Notifications
You must be signed in to change notification settings - Fork 0
fix: use threadId from worker_threads instead of workerId from workerData #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
Conversation
Summary of ChangesHello @xusd320, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors how worker thread data is accessed and managed within the Turbopack codebase. The primary goal is to enhance reliability and align with standard Node.js practices by transitioning from a custom Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request is a good refactoring that standardizes worker thread data access by using Node.js's worker_threads module instead of a custom self.workerData object. The changes correctly replace self.workerData with workerData and self.workerData.workerId with threadId. My main feedback is to re-introduce type-safety for workerData in evaluate.ts, which was lost during the refactoring. This will improve maintainability and code clarity.
| import { workerData, threadId as workerId } from 'worker_threads' | ||
|
|
||
| export type Self = DedicatedWorkerGlobalScope & { | ||
| workerData: { | ||
| workerId: number | ||
| cwd: string | ||
| binding: Binding | ||
| } | ||
| } | ||
| export type Self = DedicatedWorkerGlobalScope | ||
|
|
||
| export declare const self: Self | ||
| // @ts-ignore | ||
| const { workerId } = self.workerData | ||
|
|
||
| let binding: Binding = self.workerData.binding | ||
| let binding: Binding = workerData.binding |
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.
While moving to the standard worker_threads API is a great improvement, this change loses the type definition for workerData that was previously part of the Self type. To maintain type safety and code clarity, it's a good practice to define an interface for workerData and apply it. This also makes it clear what properties are expected, such as cwd which was present in the old type.
import { workerData as untypedWorkerData, threadId as workerId } from 'worker_threads'
interface TurbopackWorkerData {
binding: Binding
cwd: string
}
const workerData = untypedWorkerData as TurbopackWorkerData
export type Self = DedicatedWorkerGlobalScope
export declare const self: Self
let binding: Binding = workerData.binding
This pull request updates how worker thread data is accessed in the Turbopack codebase. The main change is to consistently use Node.js's
worker_threads.workerDataandworker_threads.threadIdAPIs instead of relying on a customself.workerDataobject. This improves reliability and aligns the code with standard Node.js worker thread practices.Worker thread data handling updates:
Replaced usage of
self.workerDatawith direct imports fromworker_threads(workerData,threadId) inpostcss.ts,webpack-loaders.ts, andevaluate.tsto standardize worker data access and remove custom global augmentation. [1] [2] [3]Updated worker entrypoint checks in both
postcss.tsandwebpack-loaders.tsto useworkerDatainstead ofself.workerDatafor determining if the worker should run, simplifying the worker initialization logic. [1] [2]Refactored the
Selftype inevaluate.tsto remove theworkerDataproperty, since all worker data is now accessed via theworker_threadsmodule.