Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v2.0.0-rc.0] Broken support for extended fence language #166

Closed
simonbrunel opened this issue Dec 9, 2020 · 2 comments · Fixed by GoogleForCreators/web-stories-wp#6765

Comments

@simonbrunel
Copy link

This is a regression of the feature introduced by a5d0cce.

For example, using the same input as the unit test:

```js more words are ignored
var answer = 6 * 7;
```

```js
var answer = 6 * 7;
```

Running ESLint on the markdown above:

> eslint *.md


test.md
  6:1  error  Unexpected var, use let or const instead  no-var

✖ 1 problem (1 error, 0 warnings)

Should report 2 errors with the following config:

module.exports = {
  extends: 'plugin:markdown/recommended',
  rules: {
    'no-var': 'error'
  }
};

I haven't been able to run tests locally (failed to npm install this repo on Windows) but I think that the associated unit test doesn't guarantee that the returned block will be processed by ESLint. I guess the returned block.filename is '0.js more words are ignored' which doesn't pass the glob matching on this virtual block filename.

btmills added a commit that referenced this issue Dec 16, 2020
This was working correctly in v1 but stopped working when v2 switched to
the new processor API. The generated filename was `0.js more words are
ignored`. The test case for this was insufficiently strict: it asserted
that the code block was extracted, which was sufficient with hard-coded
extensions in v1, but it didn't assert the generated name was `0.js` now
that we're taking the extension from the info string.

Per CommonMark 0.29 [1]:

> The line with the opening code fence may optionally contain some text
> following the code fence; this is trimmed of leading and trailing
> whitespace and called the info string.

I've strengthened the tests for generated filename extensions and added
a new test to ensure that leading whitespace is trimmed correctly.

[1]: https://spec.commonmark.org/0.29/#info-string
@btmills
Copy link
Member

btmills commented Dec 16, 2020

I guess the returned block.filename is '0.js more words are ignored' which doesn't pass the glob matching on this virtual block filename.

You were right! Thanks so much for the bug report. Fix is in #167.

btmills added a commit that referenced this issue Dec 16, 2020
The previous script worked on macOS and Linux, but I neglected to
consider Windows. This wouldn't affect users thankfully, only
developers, so the blast radius would have been small had it not been
caught. Now that we're testing on Windows, future bugs like this should
be caught. I'm out of ideas for doing this entirely inside npm scripts,
so I broke down and did it in JS.
@btmills
Copy link
Member

btmills commented Dec 16, 2020

I haven't been able to run tests locally (failed to npm install this repo on Windows)

Whoops. Thanks for reporting that as well. #168 should fix it.

btmills added a commit that referenced this issue Dec 20, 2020
* Chore: Test on Windows and macOS

* Fix: npm prepare script on Windows (refs #166)

The previous script worked on macOS and Linux, but I neglected to
consider Windows. This wouldn't affect users thankfully, only
developers, so the blast radius would have been small had it not been
caught. Now that we're testing on Windows, future bugs like this should
be caught. I'm out of ideas for doing this entirely inside npm scripts,
so I broke down and did it in JS.

* Chore: Increase test timeout for Windows and macOS runners

They occasionally exhibit extremely slow filesystem performance.

* Add npm-prepare script JSDoc header
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants