fix: Fix race condition when using the API (#107)

* fix: Fix race condition when using the API BREAKING CHANGE: Users consuming the API must call now `Linting().lint()`
simple-icons · Jun 7, 2024 · 85666c5 · 85666c5
1 parent 4f79345
commit 85666c5
Show file tree

Hide file tree

Showing 8 changed files with 37 additions and 45 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -33,6 +33,7 @@ jobs:
           - 16
           - 18
           - 20
+          - 22
         include:
           - os: macos-latest
             node-version: 20

diff --git a/README.md b/README.md
@@ -48,6 +48,8 @@ linting.on("done", () => {
         console.log("You've been a naughty boy!");
     }
 });
+
+linting.lint();
 ```
 
 ## Config

diff --git a/bin/cli.js b/bin/cli.js
@@ -130,6 +130,7 @@ process.on('exit', () => {
                             process.exit(EXIT_CODES.success);
                         }
                     });
+                    linting.lint();
                 })
                 .catch((error) => {
                     logger.error('Failed to lint\n', error);
@@ -171,6 +172,7 @@ process.on('exit', () => {
 
                         onLintingDone();
                     });
+                    linting.lint();
                 })
                 .catch((error) => {
                     logger.error('Failed to lint file', filePath, '\n', error);

diff --git a/src/lib/linting.js b/src/lib/linting.js
@@ -58,8 +58,6 @@ class Linting extends EventEmitter {
         this.results = {};
         /** The logger used to show debugs */
         this.logger = logging(`lint:${this.name}`);
-
-        this.lint();
     }
 
     /**

diff --git a/test/api.spec.js b/test/api.spec.js
@@ -10,43 +10,51 @@ const svg = '<svg></svg>';
 
 describe('.lintSource()', function () {
     it('should succeed without config', function (done) {
-        SVGLint.lintSource(svg).then((result) => {
-            result.on('done', () => {
-                expect(result.state).toBe(result.STATES.success);
+        SVGLint.lintSource(svg).then((linting) => {
+            linting.on('done', () => {
+                expect(linting.state).toBe(linting.STATES.success);
                 done();
             });
+            linting.lint();
         });
     });
 
     it('should succeed with empty config', function (done) {
-        SVGLint.lintSource(svg, {}).then((result) => {
-            result.on('done', () => {
-                expect(result.state).toBe(result.STATES.success);
+        SVGLint.lintSource(svg, {}).then((linting) => {
+            linting.on('done', () => {
+                expect(linting.state).toBe(linting.STATES.success);
                 done();
             });
+            linting.lint();
         });
     });
 
     it('should succeed with empty SVG', function (done) {
-        SVGLint.lintSource(svg, {}).then((result) => {
-            result.on('done', () => {
-                expect(result.state).toBe(result.STATES.success);
+        SVGLint.lintSource(svg, {}).then((linting) => {
+            linting.on('done', () => {
+                expect(linting.state).toBe(linting.STATES.success);
                 done();
             });
+            linting.lint();
         });
     });
 
     it('should succeed with empty first line', function (done) {
-        SVGLint.lintSource('\n' + svg, {}).then((result) => {
-            result.on('done', () => {
-                expect(result.state).toBe(result.STATES.success);
+        SVGLint.lintSource('\n' + svg, {}).then((linting) => {
+            linting.on('done', () => {
+                expect(linting.state).toBe(linting.STATES.success);
                 done();
             });
+            linting.lint();
         });
     });
 
     it('should throw with malformed SVG', function (done) {
-        SVGLint.lintSource('<svg<path', {}).catch(() => done());
+        SVGLint.lintSource('<svg<path', {})
+            .then((linting) => {
+                linting.lint();
+            })
+            .catch(() => done());
     });
 });
 
@@ -59,6 +67,7 @@ describe('.lintFile()', function () {
             linting.on('done', () => {
                 expect(linting.state).toBe(linting.STATES.success);
             });
+            linting.lint();
         });
     });
 
@@ -67,6 +76,7 @@ describe('.lintFile()', function () {
             linting.on('done', () => {
                 expect(linting.state).toBe(linting.STATES.success);
             });
+            linting.lint();
         });
     });
 
@@ -78,6 +88,7 @@ describe('.lintFile()', function () {
             linting.on('done', () => {
                 expect(linting.state).toBe(linting.STATES.success);
             });
+            linting.lint();
         });
     });
 
@@ -87,6 +98,7 @@ describe('.lintFile()', function () {
                 linting.on('done', () => {
                     expect(linting.state).toBe(linting.STATES.success);
                 });
+                linting.lint();
             },
         );
     });

diff --git a/test/cli.spec.js b/test/cli.spec.js
@@ -42,7 +42,7 @@ describe('CLI', function () {
         expect(failed).toBeFalsy();
     });
 
-    it('should fail with an invalid SVG', async function () {
+    it('should fail with a SVG that does not matches config', async function () {
         const {failed, exitCode} = await execCli(
             [INVALID_SVG],
             'test/projects/with-config',
@@ -60,7 +60,7 @@ describe('CLI', function () {
         expect(failed).toBeFalsy();
     });
 
-    it('should fail with an invalid SVG on stdin', async function () {
+    it('should fail with a SVG that does not matches config on stdin', async function () {
         const {failed, exitCode} = await execCli(
             ['--stdin'],
             'test/projects/with-config',

diff --git a/test/helpers.js b/test/helpers.js
@@ -30,22 +30,6 @@ export function testSucceedsFactory(svg, ruleNameOrConfig) {
                     : ruleNameOrConfig;
             const linting = await SVGLint.lintSource(svg, config);
 
-            // TODO: there is a race condition here. The this.lint() method
-            // of the Linting class is called in the constructor, so it's possible
-            // that the linting is already done before we call the on('done')
-            // event listener. Removing the next condition will make some `valid`
-            // rules tests fail.
-            if (linting.state === linting.STATES.success) {
-                resolve();
-            } else if (linting.state !== linting.STATES.linting) {
-                reject(
-                    new Error(
-                        `Linting failed (${linting.state}):` +
-                            ` ${inspect(config)}`,
-                    ),
-                );
-            }
-
             linting.on('done', () => {
                 if (linting.state === linting.STATES.success) {
                     resolve();
@@ -58,6 +42,8 @@ export function testSucceedsFactory(svg, ruleNameOrConfig) {
                     );
                 }
             });
+
+            linting.lint();
         });
     };
 }
@@ -84,18 +70,6 @@ export function testFailsFactory(svg, ruleNameOrConfig) {
                     : ruleNameOrConfig;
             const linting = await SVGLint.lintSource(svg, config);
 
-            // TODO: Same that the TODO explained at testSucceedsFactory
-            if (linting.state === linting.STATES.error) {
-                resolve();
-            } else if (linting.state !== linting.STATES.linting) {
-                reject(
-                    new Error(
-                        `Linting did not fail (${linting.state}):` +
-                            ` ${inspect(config)}`,
-                    ),
-                );
-            }
-
             linting.on('done', () => {
                 if (linting.state === linting.STATES.error) {
                     resolve();
@@ -108,6 +82,8 @@ export function testFailsFactory(svg, ruleNameOrConfig) {
                     );
                 }
             });
+
+            linting.lint();
         });
     };
 }
diff --git a/test/reporter.spec.js b/test/reporter.spec.js
@@ -8,6 +8,7 @@ async function lint(source, rules) {
         linting.on('done', () => {
             resolve(linting.results);
         });
+        linting.lint();
     });
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -33,6 +33,7 @@ jobs: @@
               - 16
               - 18
               - 20
+              - 22
             include:
               - os: macos-latest
                 node-version: 20
@@ Expand Down @@