fix(css): properly pass options to stylus compiler

[qnp]

Description

Fixes #2857

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[antfu]

Curious why Evan went with this approach. But this LGTM.

[patak-dev]

LGTM too, good find about this bug in Stylus. @qnp would it be possible to add that failing case to the playground tests so we can avoid regressions later?

[qnp]

Well, thanks @patak-js, but I'm not familiar with writing tests ( 🙈 ) so I do not really know how to do your request – even where to start. If you could provide me with some insights...

[patak-dev]

Vite's uses Jest and it aggregates the tests from the playground, running them first in dev mode, and then in build mode. To run them you need to build vite, plugin-vue first. In Vite's root:

yarn
cd packages/vite & yarn & yarn build & cd ../..
cd packages/plugin-vue & yarn & yarn build & cd ../..
yarn test

To add tests, you need to find a related playground (or create a new one). For stylus options, the css playground is a good candidate.

Each playground is a Vite app, and in the index.html markup is defined that will be later modified by the imported js files. For example, in main.js there is a test like

import less from './less.less'
text('.imported-less', less)

Where text is an util defined as

function text(el, text) {
  document.querySelector(el).textContent = text
}

The spec tests for each playground are present in /packages/playground/{name}/tests/.
Using the imported .less and modifications to the DOM listed above, the css spec includes these tests for less

test('less', async () => {
  const imported = await page.$('.less')
  const atImport = await page.$('.less-at-import')

  expect(await getColor(imported)).toBe('blue')
  expect(await getColor(atImport)).toBe('darkslateblue')
  expect(await getBg(atImport)).toMatch(isBuild ? /base64/ : '/nested/icon.png')

  editFile('less.less', (code) => code.replace('@color: blue', '@color: red'))
  await untilUpdated(() => getColor(imported), 'red')

  editFile('nested/nested.less', (code) =>
    code.replace('color: darkslateblue', 'color: blue')
  )
  await untilUpdated(() => getColor(atImport), 'blue')
})

@qnp let me know if you need more information. Adding these tests is not a requirement for this PR, but I think we should have them in the test suite. Let us know if you would like to try to add them.

[qnp]

@patak-js thanks you for taking time to explain the test playground to me. I'll thus try to author the according tests. I'll come back to you with new comits on this PR then.

Best regards,

[qnp]

Your above answer is quite precious and could be used in contibution guide. If you intend to do so,

1. please correct the commands: (&&instead of & because otherwize they are run in parallel):

yarn
cd packages/vite && yarn && yarn build && cd ../..
cd packages/plugin-vue && yarn && yarn build && cd ../..
yarn test

2. the URL shown /packages/playground/{name}/tests/. is not correct (the link is): /packages/playground/{name}/__tests__/ is OK.

[patak-dev]

Thanks for the corrections @qnp, I'll send a PR to add this to the contributing.md 👍🏼

[qnp]

@patak-js Hi,
I guess I missed something : the tests specs located in /packages/playground/css/__tests__/ are automatically generated based on index.html file ? If it is the case, how ?

I don't know how, but prettier completely messed up my specs on commit, maybe something was not properly formatted. I fixed the file now

[qnp]

@patak-js I authored the tests. While doing this job, I came across two other issues in stylus compilation:

1. preprocessor option additionalData was not used (commits ffe1d23 and 8949333)
2. preprocess option imports files are not returned as dependencies by stylus method .deps(), so I add them to the deps array so that they are watched by vite (commit 311a0aa)

[qnp]

A CI test failed: in vite package, eslint complains about:

/root/project/packages/vite/src/node/plugins/css.ts
  38:25  error  "stylus" is extraneous  node/no-extraneous-import

I don't quite understand why

[Shinigami92]

I don't quite understand why

🤔 I also checked it and it looks weird
It want's to have stylus installed as a dependency but we have @types/stylus in vite/package.json
And it isn't needed at runtime
So maybe it's a bug in eslint-plugin-node?

[qnp]

The presence of dev dependency "stylus": "^0.54.8" in packages/playground/css/package.json seems to cause the error. When I remove it, then I re-run yarn in the root of the project, vite is re-built without error (packages/vite => yarn build), otherwise the re-build fails.
But I don't understand why it complains, and not about sass or less...

[qnp]

Hi there, I'm still stuck at solving this issue. Did you have any clue based on my last comment ?

[patak-dev]

@qnp maybe you could post this PR in the #css channel in Vite Land to get a few more eyes to look into this? Is there any way to disable linting for this line if not?

[qnp]

I can disable linting for this line, yes, using // eslint-disable-line if it is acceptable for you.

[Shinigami92]

@qnp Could you update to the latest main branch? There is a new Lint CI Job 🙂

Then, if needed please use explicit disabling comments instead of ignore every lint rule for that line.

Would be nice if you could do that in two commits and let the CI fail for the first commit (merge of main) intentionally 🙂

[Shinigami92]

https://github.com/vitejs/vite/runs/2341059358#step:5:21

Oh seems that a lint step (script) is running within the prepare 🤔
I will look into that later

Thx @qnp, should not hold you off, feel free to proceed 👍

[qnp]

Done. Thanks for the help 😃

[patak-dev]

Thanks a lot for the PR @qnp ❤️