chore: Convert the worker.js file into multiple smaller files #38

scagood · 2023-07-06T20:54:53Z

This does the following:

Adds a .editorconfig
Moves worker.js to worker/worker.js
Abstracts away the message response, logging and error handling

Makes the worker using the following structure:

Filename	Description
`convert-results.js`	Converts `ESLint.LintResult[]` into atom-linter results
`errors.js`	Holds all (known) emitted errors
`eslint.js`	Retrieves the ESLint instance and manages caching
`fs-utils.js`	Store the file searching tools
`messaging.js`	Abstracts away the event messaging
`worker.js`	The main worker entry point

savetheclocktower · 2023-07-06T21:00:36Z

On first glance this seems fine, but was there a pressing need to break up a file that was under 500 LOC?

spec/linter-eslint-node-spec.js

scagood · 2023-07-06T20:58:18Z

spec/fixtures/files/config-change/.eslintrc.js

I split this off because it was causing conflicts in other tests

scagood · 2023-07-06T21:00:02Z

.eslintrc

+    // "comma-dangle": ["error", "never"],
+    // "quotes": ["error", "double"],


Wanted to touch base on this one, I think these two rules make sense, if not I can remove them.

The rest of the changes here are just whitespace

scagood · 2023-07-06T21:02:04Z

lib/worker/messaging.js

+    if (
+      (start === '[' && end === ']') ||
+      (start === '{' && end === '}') ||
+      (start === '"' && end === '"')
+    ) {
+      return input;
+    }


This is a super simple 'is this string json already' check, if its not, we stringify it anyway! I was not sure of a better way to do this 🤔

scagood · 2023-07-06T21:04:45Z

lib/worker/messaging.js

+function runWith (data, handler) {
+  store.run(
+    {
+      key: data?.key,
+      type: data?.type,
+      filePath: data?.filePath,
+      projectPath: data?.projectPath,
+    },
+    handler,
+  );
+}


This allows any messages or errors thrown to have the requests context in the response.
This is useful when its an uncaught exception.

scagood · 2023-07-06T21:06:42Z

lib/worker/worker.js

I am not sure I like how this file looks or works... I am considering moving the jobs to their own folder, like:
worker/worker.js
worker/jobs/:job.js
worker/utilites/:utility.js

I think that would make things even clearer

scagood · 2023-07-06T21:08:01Z

lib/worker/worker.js

+    Messaging.runWith(data, async () => {
+      try {
+        if (!data.key) {
+          throw new JobKeyError();
+        }
+
+        Messaging.emit(await processMessage(data));
+      } catch (error) {
+        Messaging.emitError(handleError(error, data));
+      }
+    });


This is now effectivly the only place to respond to requests. It allows for more consise response handling.

All jobs now just return what the job wants to output, or throws an error.

scagood · 2023-07-06T21:08:39Z

lib/worker/worker.js

+    try {
+      Messaging.emitError(error);
+    } finally {
+      process.exit(1);
+    }


I am not sure we need this anymore, but I wanted to leave this in as I don't want to find out the hard way with a ticket later 😂

scagood · 2023-07-06T21:09:55Z

@savetheclocktower I am gearing up torwards the FlatESLint class 😀
https://eslint.org/blog/2022/08/new-config-system-part-3

savetheclocktower · 2023-07-06T21:41:07Z

Wow, that was not on my radar. Much appreciated.

I don't know if you're any good with CI stuff, but our jobs need retrofitting to work with Pulsar. @UziTech pointed me to setup-pulsar a while back, but I haven't had time to investigate, because I'm spending a lot of time working on Pulsar's core. I haven't even gotten far enough to ascertain whether it's mature enough for us to be able to do a simple swap for setup-atom.

If you're willing to give this a try, it would be a huge help; otherwise we're in a position where we have to make disruptive changes to the codebase without being able to rely on CI. If it's something you despise doing, then I can give it my best shot.

scagood · 2023-07-06T21:42:00Z

I can certainly give it a shot 😄
I will set up all the actions on my fork to test I think. May not have time till the weekend though

savetheclocktower · 2023-07-06T22:16:49Z

Strangely, all tests pass when I run pulsar --test spec from the command line, but I get seven failures when I run Run Package Specs from the GUI. Annoying, but not a deal-breaker.

The test you skipped works fine for me locally if I un-skip it. If increasing the timeout helps the test pass for you locally, let's try that.

scagood · 2023-07-07T12:43:13Z

Draft CI found in #39
Not sure why installing other packages did not work out 🤔

scagood · 2023-07-11T21:25:42Z

mmm, rebase tests failed, will look later 😂

scagood · 2023-07-12T11:22:00Z

lib/job-manager.js

+    if (worker == null) {
+      worker = this.worker;
+    }


Not 100% sure why this worked, I think it's babel doing its thing 🤔

https://node.green/#ES2021-features-Logical-Assignment-----basic-support

scagood · 2023-07-12T11:23:57Z

lib/node-path-tester.js

+      let stdout = execSync(
+        `${bin} --version`,
+        {
+          stdio: [
+            'ignore', // Ignore stdin
+            undefined, // return stdout
+            'ignore' // Ignore stderr
+          ]
+        }
+      );


This fixes the random logging of /bin/sh asdasdas not found

scagood · 2023-07-12T20:11:32Z

Urm, can someone better versed in windows help 👀

I am so lost, I cant get the tests to fail 😬

savetheclocktower · 2023-07-12T21:37:25Z

I'm on macOS, so I'll be of no help, sadly.

scagood · 2023-07-12T21:47:31Z

I now have all major OS distros installed, my daily driver is also mac 😂

scagood commented Jul 6, 2023

View reviewed changes

savetheclocktower mentioned this pull request Jul 11, 2023

ci: use pulsar in ci #39

Merged

scagood force-pushed the main branch from 2e29f1f to e47b643 Compare July 11, 2023 21:21

scagood force-pushed the main branch from e47b643 to 4a5e416 Compare July 12, 2023 11:20

scagood commented Jul 12, 2023

View reviewed changes

scagood force-pushed the main branch from e1f3f0f to 5610ef4 Compare July 12, 2023 21:46

scagood closed this Aug 9, 2023

scagood force-pushed the main branch from 5610ef4 to d6587c5 Compare August 9, 2023 09:08

		// "comma-dangle": ["error", "never"],
		// "quotes": ["error", "double"],

chore: Convert the worker.js file into multiple smaller files #38

chore: Convert the worker.js file into multiple smaller files #38

Uh oh!

Conversation

scagood commented Jul 6, 2023

Uh oh!

savetheclocktower commented Jul 6, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scagood commented Jul 6, 2023

Uh oh!

savetheclocktower commented Jul 6, 2023

Uh oh!

scagood commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

savetheclocktower commented Jul 6, 2023

Uh oh!

scagood commented Jul 7, 2023

Uh oh!

scagood commented Jul 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scagood commented Jul 12, 2023

Uh oh!

savetheclocktower commented Jul 12, 2023

Uh oh!

scagood commented Jul 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

scagood commented Jul 6, 2023 •

edited

Loading