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

fix(mocha): workaround the dart-sass stylesheet load issue #6444

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

fangbinwei
Copy link
Collaborator

@fangbinwei fangbinwei commented Apr 26, 2021

TODO

  • test

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Underlying tools
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Other information:

check #6427 (comment)

It's a hackish workaround, feel free to close it if this is not accepted.

Fixes #6427, Fixes #4053

@fangbinwei fangbinwei marked this pull request as ready for review April 28, 2021 13:30
@haoqunjiang
Copy link
Member

How about ignoring these style files in the unit test plugin?
I think they are not actually tested anyway. Maybe that's less error-prone.

@lambertkevin
Copy link

How about ignoring these style files in the unit test plugin?
I think they are not actually tested anyway. Maybe that's less error-prone.

I might be mixing up things, but how would you test css-based things like wrapper.isVisible() with vue-test-utils if visibility, display or opacity comes from an @import/@use or from an @extends that needs this import ?

@haoqunjiang
Copy link
Member

I might be mixing up things, but how would you test css-based things like wrapper.isVisible() with vue-test-utils if visibility, display or opacity comes from an @import/@use or from an @extends that needs this import ?

Oops. I forgot about that feature.
But I'm wondering if it should be supported at all.

Because the tests are run in Node.js environment with JSDOM, the .isVisible method does not support visibility changes triggered by class styles. It is a known limitation of JSDOM: vuejs/vue-test-utils#1456 So the use case is very limited.

And, the default Jest presets provided by Vue CLI never support such usage (.css files are stubbed in the Jest presets).

@lambertkevin
Copy link

Oops. I forgot about that feature.
But I'm wondering if it should be supported at all.

Because the tests are run in Node.js environment with JSDOM, the .isVisible method does not support visibility changes triggered by class styles. It is a known limitation of JSDOM: vuejs/vue-test-utils#1456 So the use case is very limited.

And, the default Jest presets provided by Vue CLI never support such usage (.css files are stubbed in the Jest presets).

Oh didn't know that. Then yeah, you're right, the whole style block of an SFC should simply be ignored.
I don't even know why it wasn't the case from the very beginning since it cannot be interpreted in any way by JSDOM ?

@germandb
Copy link

I just update my VUE APP to use Dart-Sass and I get this error on all my test because I use a submodule with all my style-sheets there. Any news aproving this PR or @sodatea the ignore all styles aproche got implemented some where?.

@fangbinwei

This comment has been minimized.

@fangbinwei
Copy link
Collaborator Author

How about ignoring these style files in the unit test plugin? I think they are not actually tested anyway. Maybe that's less error-prone.

Maybe style files can not simply be ignored, since style modules related codes will throw error without these style files.
vue-jest can handle this case.

case:

<template>
  <div :class="$style.hello">
  </div>
</template>

<style lang="scss" module>
.hello {
  display: none;
}
</style>

@ekampp
Copy link

ekampp commented Feb 8, 2022

Hey.

Thanks for everyones great work on this! I have now also run into this problem. What's the horizon for merging this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants