NEW(esint): @W-18695515@ Manually resolve plugin conflicts … #304
Conversation
…d events from background worker
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| async function dynamicallyImport(absJavaScriptFilePath: string): Promise<any> { | ||
| // To avoid issues with dynamically importing absolute paths on Windows, we need to convert to url with pathToFileURL. | ||
| const moduleUrl: string = pathToFileURL(absJavaScriptFilePath).href; |
There was a problem hiding this comment.
While working on this PR, I realized the more robust way to do dynamic imports is to use the pathToFileURL utility instead of manually replacing slashes for windows.
| */ | ||
| export abstract class Engine { | ||
| export abstract class EngineEventEmitter { |
There was a problem hiding this comment.
I split up the Engine class into 2 abstract classes so we can very easily reuse the event emitter methods for helper instances inside of an Engine. This made everything super clean instead of passing around function handles.
| // Calculate configs for files | ||
| const calculatedConfigs: Linter.Config[] = await Promise.all(context.filesToScan.map( | ||
| f => eslint.calculateConfigForFile(f) as Linter.Config)); | ||
| async calculateESLintContext(engineConfig: ESLintEngineConfig, eslintWorkspace: ESLintWorkspace, userConfigFile?: string): Promise<ESLintContext> { |
There was a problem hiding this comment.
The implementation details did not change at all... just moved the function inside of a class so that we could make it an event emitter and forward the events from the eslint factory.
| worker.on('message', (msg: Event | {type: "output", output: Output}) => { | ||
| if (msg.type === "output") { | ||
| resolve(msg.output); | ||
| } else { | ||
| this.emitEvent(msg); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Super elegant in my opinion! We can can forward all events from background worker threads to the main thread very cleanly.
| ` for (const eventType of Object.values(engineApi.EventType)) {\n` + | ||
| ` task.onEvent(eventType, (evt) => {\n` + | ||
| ` parentPort.postMessage(evt);\n` + | ||
| ` });\n` + | ||
| ` }\n` + |
There was a problem hiding this comment.
Where the magic happens. :-)
Co-authored-by: Juliet Shackell <[email protected]>
stephen-carter-at-sf
force-pushed
the
sc/eslintUpdates
branch
from
5f61d8f to
b2f51b7
Compare
| @@ -626,6 +604,53 @@ describe('Typical tests for the runRules method of ESLintEngine', () => { | |||
| expect(results.violations).toContainEqual(expectedTsViolation_noInvalidRegexp); | |||
| expect(results.violations).toContainEqual(expectedTsViolation_noWrapperObjectTypes); | |||
| }); | |||
|
|
|||
| it('When workspace contains custom config that installs same plugin as one of our base plugins, we resolve plugins, describe and run rules properly', async () => { | |||
| if (os.platform().startsWith('win')) { | |||
There was a problem hiding this comment.
I'm fine with us disabling the test on Windows for runtime reasons, but I do want to be sure that you actually ran it at least once on Windows to make sure it passes?
There was a problem hiding this comment.
Done manually just now - yes it passes.
| @@ -740,3 +740,12 @@ function wasFieldSuppliedByUser(engineOverrides: EngineOverrides, fieldName: str | |||
| function findCaseInsensitiveKey(obj: object, key: string): string | undefined { | |||
| return Object.keys(obj).find(k => k.toLowerCase() === key.toLowerCase()); | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| async function dynamicallyImport(absJavaScriptFilePath: string): Promise<any> { | |||
There was a problem hiding this comment.
Is the explicit any here truly necessary? Could you use an as-cast in the return statement?
There was a problem hiding this comment.
Unfortunately no - the any is necessary. I tried even using unknown but we need to do post import validation that needs it to be any sadly.
… and forward events from background worker