fix: add syntax entry points to fix lazy load bug #238

adalinesimonian · 2021-09-29T00:23:00Z

Fixes #237

The bundler cannot recognize the requires in the stylelint library for syntaxes, due to the lazy loads in the form importLazy('package') obscuring the require that is taking place.

This patch adds each syntax as its own entry point to bypass this limitation. It will no longer be necessary when targeting stylelint 14.

see: https://github.com/stylelint/stylelint/blob/13.13.1/lib/syntaxes/index.js

Fixes #237 The bundler cannot recognize the requires in the stylelint library for syntaxes, due to the lazy loads in the form `importLazy('package')` obscuring the require that is taking place. This patch adds each syntax as its own entry point to bypass this limitation. It will no longer be necessary when targeting stylelint 14. see: https://github.com/stylelint/stylelint/blob/13.13.1/lib/syntaxes/index.js

ntwb · 2021-09-29T01:12:22Z

The tests are passing in CI, but I noticed this warning: (same on all 3 CI platforms)

Looking at the CI run, I see:


 > index.js:19:36: warning: "./server.js" should be marked as external for use with "require.resolve"
    19 │   const serverPath = require.resolve('./server.js');
       ╵                                      ~~~~~~~~~~~~~

1 warning

  dist/server.js                  4.0mb ⚠️
  dist/syntax-css-in-js.js        2.4mb ⚠️
  dist/syntax-markdown.js         1.4mb ⚠️
  dist/syntax-html.js             1.2mb ⚠️
  dist/syntax-sass.js           858.5kb
  dist/index.js                 425.4kb
  dist/syntax-sugarss.js        259.8kb
  dist/syntax-scss.js           248.9kb
  dist/syntax-less.js           240.1kb
  dist/server.js.map              6.4mb
  dist/syntax-css-in-js.js.map    3.7mb
  dist/syntax-markdown.js.map     2.0mb
  dist/syntax-html.js.map         1.7mb
  dist/syntax-sass.js.map         1.3mb
  dist/index.js.map             732.4kb
  dist/syntax-sugarss.js.map    433.8kb
  dist/syntax-scss.js.map       421.5kb
  dist/syntax-less.js.map       413.6kb

adalinesimonian · 2021-09-29T01:18:06Z

The tests are passing in CI, but I noticed this warning: (same on all 3 CI platforms)
 > index.js:19:36: warning: "./server.js" should be marked as external for use with "require.resolve"
    19 │   const serverPath = require.resolve('./server.js');

I encountered this warning when initially configuring esbuild; the issue is that if you add server.js as an external dependency, the build errors out because it is also an entry point. I didn't see a way to configure different external dependencies per entry point, and it doesn't look like this warning results in any problematic behaviour.

ntwb · 2021-09-29T01:42:16Z

I encountered this warning when initially configuring esbuild; the issue is that if you add server.js as an external dependency, the build errors out because it is also an entry point. I didn't see a way to configure different external dependencies per entry point, and it doesn't look like this warning results in any problematic behaviour.

Thanks, I'm fine with this...

I'll update my local and run the tests locally on some test repos I have this PR

Hopefully this will help confirm that a follow up release works as expected

Will report back here shortly

ntwb · 2021-09-29T02:13:43Z

Running npm install -> npm test on this branch:

Test Suites: 2 failed, 13 passed, 15 total
Tests:       3 failed, 43 passed, 46 total
Snapshots:   1 failed, 1 obsolete, 36 passed, 37 total
Time:        177.468 s
Ran all test suites in 2 projects.
npm ERR! Test failed.  See above for more details.

I also get some VS Code warnings on each of the tests, these can be ignored, I think:

Warning: 'sandbox' is not in the list of known options, but still passed to Electron/Chromium.
Warning: 'disable-workspace-trust' is not in the list of known options, but still passed to Electron/Chromium.
2021-09-29 12:05:12.516 Code Helper (Renderer)[71303:1207720] CoreText note: Client requested name ".NewYork-Regular", it will get Times-Roman rather than the intended font. All system UI font access should be through proper APIs such as CTFontCreateUIFontForLanguage() or +[NSFont systemFontOfSize:].
DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead. (Use `Code Helper (Renderer) --trace-deprecation ...` to show where the warning was created)

The test errors are:

1.

 FAIL  test/workspace/ws-validate-test/index.js (6.47 s)
  ● vscode-stylelint with "stylelint.validate" › should be ignored lint and autofix on css

    expect(received).toEqual(expected) // deep equality

    - Expected  -  1
    + Received  + 26

    - Array []
    + Array [
    +   Object {
    +     "code": Object {
    +       "target": Object {
    +         "$mid": 1,
    +         "authority": "stylelint.io",
    +         "path": "/user-guide/rules/indentation",
    +         "scheme": "https",
    +       },
    +       "value": "indentation",
    +     },
    +     "message": "Expected indentation of 4 spaces (indentation)",
    +     "range": Array [
    +       Object {
    +         "character": 2,
    +         "line": 2,
    +       },
    +       Object {
    +         "character": 2,
    +         "line": 2,
    +       },
    +     ],
    +     "severity": "Error",
    +     "source": "stylelint",
    +   },
    + ]

      32 |
      33 |              // Check the result.
    > 34 |              expect(getStylelintDiagnostics(cssDocument.uri)).toEqual([]);
         |                                                               ^
      35 |
      36 |              // Execute the Autofix command.
      37 |              await commands.executeCommand('stylelint.executeAutofix');

      at Object.<anonymous> (ws-validate-test/index.js:34:52)

  ● vscode-stylelint with "stylelint.validate" › should be ignored lint and autofix on markdown

    expect(received).toEqual(expected) // deep equality

    - Expected  -  1
    + Received  + 26

    - Array []
    + Array [
    +   Object {
    +     "code": Object {
    +       "target": Object {
    +         "$mid": 1,
    +         "authority": "stylelint.io",
    +         "path": "/user-guide/rules/indentation",
    +         "scheme": "https",
    +       },
    +       "value": "indentation",
    +     },
    +     "message": "Expected indentation of 4 spaces (indentation)",
    +     "range": Array [
    +       Object {
    +         "character": 2,
    +         "line": 4,
    +       },
    +       Object {
    +         "character": 2,
    +         "line": 4,
    +       },
    +     ],
    +     "severity": "Error",
    +     "source": "stylelint",
    +   },
    + ]

      70 |
      71 |              // Check the result.
    > 72 |              expect(getStylelintDiagnostics(mdDocument.uri)).toEqual([]);
         |                                                              ^
      73 |
      74 |              // Execute the Autofix command.
      75 |              await commands.executeCommand('stylelint.executeAutofix');

      at Object.<anonymous> (ws-validate-test/index.js:72:51)

 › 1 snapshot obsolete.

2.

FAIL  test/workspace/ws-syntax-test/index.js
  ● vscode-stylelint with "stylelint.syntax" › should work even if "stylelint.syntax" is defined

    expect(received).toMatchSnapshot()

    Snapshot name: `vscode-stylelint with "stylelint.syntax" should work even if "stylelint.syntax" is defined 1`

    - Snapshot  - 9
    + Received  + 2

    @@ -1,16 +1,9 @@
      Array [
        Object {
    -     "code": Object {
    -       "target": Object {
    -         "authority": "stylelint.io",
    -         "path": "/user-guide/rules/indentation",
    -         "scheme": "https",
    -       },
    -       "value": "indentation",
    -     },
    -     "message": "Expected indentation of 4 spaces (indentation)",
    +     "code": "CssSyntaxError",
    +     "message": "Unknown word (CssSyntaxError)",
          "range": Object {
            "end": Object {
              "character": 2,
              "line": 2,
            },

      21 |              const diagnostics = getStylelintDiagnostics(cssDocument.uri);
      22 |
    > 23 |              expect(diagnostics.map(normalizeDiagnostic)).toMatchSnapshot();
         |                                                           ^
      24 |      });
      25 | });
      26 |

      at Object.<anonymous> (ws-syntax-test/index.js:23:48)

 › 1 snapshot failed.

Unsure why these fail locally, will read and look a little closer

ota-meshi

Thank you for the quick fix.

ntwb · 2021-09-29T02:20:27Z

I've just run the above again on my laptop, same results.

The above results were on an M1 Mac, these additional results on an Intel Mac

adalinesimonian · 2021-09-29T02:23:10Z

@ntwb This discrepancy in test results between running tests in the project directory and elsewhere makes me wonder if we should find a way to have the tests enforce testing against the bundled stylelint and a local copy? If that had been the case, I think this whole bug would have been avoided entirely. That, or removing the bundled stylelint entirely per #36.

ota-meshi · 2021-09-29T02:29:48Z

I've tested it locally and everything works fine. I'm using macOS.

ota-meshi · 2021-09-29T02:32:53Z

If you have a globally installed stylelint, this extension will use it, so there may be differences in behavior. I don't have stylelint installed globally.

adalinesimonian · 2021-09-29T02:34:38Z

I've tested it locally and everything works fine. I'm using macOS.

Same here.

@ntwb I have a feeling that the errors you're running into might be independent of this specific fix; I'm not at my computer right at this moment so I can't check for certain. Earlier, I made a copy of the syntax test folder and ran VS Code in it with this fix in it and the behaviour seemed to be what it should be, or at the very least what it was before the bundling update.

adalinesimonian · 2021-09-29T02:38:18Z

In the meantime, should we merge this and throw out a patch release? I think we should make an issue or two to address the tests not covering all real-life scenarios or having different results in different contexts.

ntwb · 2021-09-29T03:11:00Z

I'm just testing the extension locally now, in the meantime can we get an entry added to the changelog for this please, as the changelog is linked from https://marketplace.visualstudio.com/items?itemName=stylelint.vscode-stylelint

Edit: Ignore the above, can do it after the PR is merged and release shipped

ntwb · 2021-09-29T03:23:37Z

Testing locally having run vsce package 0.99.9 (A fake version number for local testing:

✅ SCSS Test

✅ CSS Test

ntwb

💯

adalinesimonian · 2021-09-29T03:35:44Z

Merged, bumped version to 0.87.3, updated changelog, cut a release, and published to the marketplace!

ntwb · 2021-09-29T03:41:57Z

✅ Testing v0.86.3

Same results per local tests above, all good, thanks @adalinesimonian

adalinesimonian added the priority: high label Sep 29, 2021

adalinesimonian requested a review from ota-meshi September 29, 2021 00:23

adalinesimonian self-assigned this Sep 29, 2021

ota-meshi approved these changes Sep 29, 2021

View reviewed changes

ntwb approved these changes Sep 29, 2021

View reviewed changes

adalinesimonian merged commit d65c986 into master Sep 29, 2021

adalinesimonian deleted the fix-lazy-load branch September 29, 2021 03:30

adalinesimonian mentioned this pull request Sep 29, 2021

fix: use postcss-syntax fork until 1.x #240

Merged

adalinesimonian added For Uncommitted Issue and removed Impact: Fails to Run labels Oct 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add syntax entry points to fix lazy load bug #238

fix: add syntax entry points to fix lazy load bug #238

adalinesimonian commented Sep 29, 2021

ntwb commented Sep 29, 2021 •

edited

Loading

adalinesimonian commented Sep 29, 2021

ntwb commented Sep 29, 2021

ntwb commented Sep 29, 2021

ota-meshi left a comment

ntwb commented Sep 29, 2021

adalinesimonian commented Sep 29, 2021

ota-meshi commented Sep 29, 2021

ota-meshi commented Sep 29, 2021

adalinesimonian commented Sep 29, 2021

adalinesimonian commented Sep 29, 2021

ntwb commented Sep 29, 2021 •

edited

Loading

ntwb commented Sep 29, 2021

ntwb left a comment

adalinesimonian commented Sep 29, 2021

ntwb commented Sep 29, 2021

fix: add syntax entry points to fix lazy load bug #238

fix: add syntax entry points to fix lazy load bug #238

Conversation

adalinesimonian commented Sep 29, 2021

ntwb commented Sep 29, 2021 • edited Loading

adalinesimonian commented Sep 29, 2021

ntwb commented Sep 29, 2021

ntwb commented Sep 29, 2021

1.

2.

ota-meshi left a comment

Choose a reason for hiding this comment

ntwb commented Sep 29, 2021

adalinesimonian commented Sep 29, 2021

ota-meshi commented Sep 29, 2021

ota-meshi commented Sep 29, 2021

adalinesimonian commented Sep 29, 2021

adalinesimonian commented Sep 29, 2021

ntwb commented Sep 29, 2021 • edited Loading

ntwb commented Sep 29, 2021

✅ SCSS Test

✅ CSS Test

ntwb left a comment

Choose a reason for hiding this comment

adalinesimonian commented Sep 29, 2021

ntwb commented Sep 29, 2021

✅ Testing v0.86.3

ntwb commented Sep 29, 2021 •

edited

Loading

ntwb commented Sep 29, 2021 •

edited

Loading