fix(ruleset-bundler): builtin module should not be treated as external

[jianyexi]

Fixes case 2 in #2107

Checklist

• Tests added / updated
• Docs added / updated

Does this PR introduce a breaking change?

• Yes
• No

If indicated yes above, please describe the breaking change(s).

Remove this quote before creating the PR.

Screenshots

If applicable, add screenshots or gifs to help demonstrate the changes. If not applicable, remove this screenshots section before creating the PR.

Additional context

Add any other context about the pull request here. Remove this section if there is no additional context.

[P0lip]

Could you please add a test case for that?
We already have a few in there, so you can either try adapting it or just create a new one.

[P0lip]

albeit tbh we should probably simply register builtins plugins if we don't do it yet 🤔
I'll get back to you on that when I take a look.

[jianyexi]

albeit tbh we should probably simply register builtins plugins if we don't do it yet 🤔 I'll get back to you on that when I take a look.

The issue is that if spectral-cli bundles a remote ruleset, it will deem the imported package like '@stoplight/spectral-rulesets' as external package and fails, and if replacing it to a cdn url like "https://cdn.jsdelivr.net/npm/@stoplight/spectral-rulesets/+esm" , it also has issue . I already described it in #2107.
I do want spectral-cli can support loading builtin modules when bundling a remote url ruleset

[jianyexi]

Could you please add a test case for that? We already have a few in there, so you can either try adapting it or just create a new one.

already added

[jianyexi]

@P0lip Kindly reminder, as this issue is blocking us using spectral in our pipeline.

[P0lip]

Hey, my apologies for the delay.

The test you added works even without the change around externals. This is thanks to the use of builtins plugin, and I'd say that's the workaround here is to simply use that plugin if needed.
I'm rather hesitant to consider builtins as local since they might be external if serve your ruleset over HTTPS in the context of a browser.
Not directly related to the PR, but we should probably try to satisfy CDNs, so that the assets can be built correctly

[jianyexi]

Thanks for reviewing. It's weird, the unit test is similar to the usage in

spectral/packages/cli/src/services/linter/utils/getRuleset.ts

Line 66 in 88e1a4b

ruleset = await bundleRuleset(rulesetFile, {

, but the result is different with the case 2 in #2107, do you know why? (Perhaps the test context is different with the real CLI context?)

[jianyexi]

Since my change is for the scenarios of the CLI or node, I think it will not impact the browser users.
BTW can you tell me more about satisfying CDNs? Why the spectral can't satisfy CDNs now? @P0lip

[jianyexi]

@P0lip I resolved the test case issue, after adding the 'format:commonjs', this case can cover the case2 in the issue #2107

[P0lip]

Since my change is for the scenarios of the CLI or node, I think it will not impact the browser users.

Node.js has a way to load modules over https, and they clearly state that loading local resources is disallowed in such a case - see "Cannot load non-network dependencies".

BTW can you tell me more about satisfying CDNs? Why the spectral can't satisfy CDNs now

Skypack seems to bail out on a particular Ajv import, while jsdelivr isn't happy with lodash import @stoplight/json has out there.

@P0lip I resolved the test case issue, after adding the 'format:commonjs', this case can cover the case2 in the issue #2107

Taking a look now

[P0lip]

I took a look at the new test, and everything still works as expected.
It's the expected output that's incorrect when builtins plugin is registered.
builtins plugin inserts globalThis[Symbol.for('${NAME}')]['${instanceId}']['${id}'] and sets the equivalent property on globalThis so it can be accessed later on.

Here's an updated test showing that the code can be executed properly

diff --git a/packages/ruleset-bundler/src/__tests__/index.jest.test.ts b/packages/ruleset-bundler/src/__tests__/index.jest.test.ts
index 4c92dac4..2ed04945 100644
--- a/packages/ruleset-bundler/src/__tests__/index.jest.test.ts
+++ b/packages/ruleset-bundler/src/__tests__/index.jest.test.ts
@@ -128,21 +128,23 @@ rules: {
       plugins: [builtins(), commonjs(), ...node({ fs, fetch }), virtualFs(io)],
     });
 
-    expect(code).toContain(`var input = {
-extends: [spectralRulesets.oas],
-rules: {
-  'my-rule': {
-    given: '$',
-    then: {
-      function: spectralFunctions.schema,
-      functionOptions: {
-        schema: {
-          type: 'object',
+    const m: { exports?: unknown } = {};
+    Function('module', code)(m);
+    expect(m.exports).toEqual({
+      extends: [(await import('@stoplight/spectral-rulesets')).oas],
+      rules: {
+        'my-rule': {
+          given: '$',
+          then: {
+            function: (await import('@stoplight/spectral-functions')).schema,
+            functionOptions: {
+              schema: {
+                type: 'object',
+              },
+            },
+          },
         },
       },
-    },
-  },
-},
-};`);
+    });
   });
 });
diff --git a/packages/ruleset-bundler/src/index.ts b/packages/ruleset-bundler/src/index.ts
index e4acb8c2..38acec50 100644
--- a/packages/ruleset-bundler/src/index.ts
+++ b/packages/ruleset-bundler/src/index.ts
@@ -36,10 +36,7 @@ export async function bundleRuleset(
         : target === 'browser'
         ? id => isURL(id)
         : (id, importer) =>
-            id.startsWith('node:') ||
-            (!isURL(id) &&
-              isPackageImport(id) &&
-              (importer === void 0 || !isURL(importer) || id.startsWith('@stoplight/spectral-'))),
+            id.startsWith('node:') || (!isURL(id) && isPackageImport(id) && (importer === void 0 || !isURL(importer))),
   });
 
   return (await bundle.generate({ format: format ?? (target === 'runtime' ? 'iife' : 'esm'), exports: 'auto' }))

I'm still uncertain what exactly you're trying to achieve. Perhaps you could try to give me a ruleset you try to use?

[jianyexi]

@P0lip what I want to fix is the CLI issue described in #2107 (comment), it's blocking us to run lint through CLI for a given remote ruleset which imports built in modules

[jianyexi]

Sorry for updating the test case again, the previous version test has a subtle difference with the exact issue I encountered, I think it's caused by the commonjs statement module.exports = ruleset

[mikekistler]

I'm not clear on what the issue is with the tests, but I would like to chime in to say that I just recreated the problem using the current develop branch and then verified that this PR fixes it.

Using this ruleset file:

extends:
  - https://raw.githubusercontent.com/azure/azure-openapi-validator/develop/packages/rulesets/generated/spectral/az-common.js
  - https://raw.githubusercontent.com/azure/azure-openapi-validator/develop/packages/rulesets/generated/spectral/az-dataplane.js
rules:
  az-enum-insteadOf-boolean: off

here is the behavior I get when running with the current develop branch:

[mikekistler@Mikes-MacBook-Pro] ~/Projects/stoplightio/spectral (develop)>node packages/cli/dist/index.js lint -r aov-gh.yaml $f
Error running Spectral!
Use --verbose flag to print the error stack.
Error #1: '@stoplight/spectral-formats' is imported as an external by @stoplight/spectral-formats?commonjs-proxy, but is already an existing non-external module id.
[!] Error: unfinished hook action(s) on exit:
(@stoplight-spectral/url) load "https://raw.githubusercontent.com/azure/azure-openapi-validator/develop/packages/rulesets/generated/spectral/az-common.js"

Running with the fix in this PR, the linter runs successfully:

[mikekistler@Mikes-MacBook-Pro] ~/Projects/stoplightio/spectral (dev/fix-bundle)>node packages/cli/dist/index.js lint -r aov-gh.yaml $f                                                                                               

/Users/mikekistler/Projects/Azure/azure-rest-api-specs/specification/iotcentral/data-plane/Microsoft.IoTCentral/preview/1.1-preview/iotcentral.json
   6:16         error  az-version-convention           API version should be a date in YYYY-MM-DD format, optionally suffixed with '-preview'.       info.version
  30:19       warning  az-schema-description-or-title  Schema should have a description or title.                                                    definitions.ADGroupUser
  30:19   information  az-schema-names-convention      Schema name should be Pascal case.                                                            definitions.ADGroupUser
  53:16       warning  az-schema-description-or-title  Schema should have a description or title.                                                    definitions.ApiToken

...

 4798:15      warning  az-parameter-names-convention   Parameter name "$top" should not begin with '$' or '@'.                                       parameters.top.name

✖ 271 problems (1 error, 244 warnings, 26 infos, 0 hints)

[P0lip]

Ah, got it. I see now. So it's commonjs plugin messing things up.

[P0lip]

Thank you all for the feedback as well as the great test! I've provided a slightly fix over here #2174

[jianyexi]

Thank you all for the feedback as well as the great test! I've provided a slightly fix over here #2174

Looks great, thanks so much

[mnaumanali94]

Fixed in #2174