-
Notifications
You must be signed in to change notification settings - Fork 156
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: allow importing relative paths in global resources #373
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow reply. Let's get this merged and released.
Are you able to resolve the conflicts?
@lmiller1990 I can't unfortunately. It is actually on the latest v4 code, but there is no FYI: |
af32ba1
to
a3da7fc
Compare
Sorry about the delay on this. If we can resolve the conflicts, we can merge! |
@lmiller1990 It seems like you missed my explanation about the source of the conflicts. In #374 (comment), you confirmed that but there is no TLDR;To fix the conflicts, you need to create a |
Right, sorry, I did not explain the context. In order to support both Vue 2 and Vue 3 (and Jest 26.x and 27.x) we moved to a monorepo. There are two packages: We have two branches:
The code for v4, which works with Vue 2, is in Ideally we want to be patching the latest version of Jest, which is v27, but if you are still on v26 you could submit the patch against that branch/package and we could release it. Sorry if that's confusing, if it's still not clear, let me know. |
Hi! Can you resolve the conflicts? Then we can get this merged up. Let me know if my above comment is confusing. |
@lmiller1990 I'm sorry, I'm currently on vacation, but I'm back on Monday and can resolve the conflicts on the I'll keep you posted. |
a3da7fc
to
d34db83
Compare
@lmiller1990 I was able to resolve the conflicts by rebasing my branch on I also noticed something wrong in the e2e package.json files: it refers to I decided to go ahead and fix the e2e test run in 4222d23 using the local versions like I suggested. It's out of scope so let me know if you would rather have it in a separate PR. If you like it, I'm happy to submit a new PR to implement it for |
@@ -32,6 +40,7 @@ function extractClassMap(cssCode) { | |||
function getPreprocessOptions(lang, filePath, jestConfig) { | |||
if (lang === 'scss' || lang === 'sass') { | |||
return { | |||
filename: filePath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After rebasing, all the tests for vue files including a relative @import
statement started to fail when I was testing vue-jest
in my own project. This line fixed the issue which provide the file path to the processor (sass
) making it able to resolve the relative paths.
aeacd25
to
4222d23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some q's! Thanks.
@@ -12,14 +11,23 @@ function getGlobalResources(resources, lang) { | |||
let globalResources = '' | |||
if (resources && resources[lang]) { | |||
globalResources = resources[lang] | |||
.map(resource => path.resolve(process.cwd(), resource)) | |||
.filter(resourcePath => fs.existsSync(resourcePath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot what this does exactly but it looks like it does something different to the code you added. It looks like this line handles <style src="/something.css">
. Did I misunderstand this? Do you know what is going on here by any chance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can provide more details indeed. This function is only used to inject the global resources provided in the jest config. If you check where it is being used:
let content =
getGlobalResources(vueJestConfig.resources, stylePart.lang) +
stylePart.content
It basically define the final content of the <style>
block by prepending the global resources but only if there are resources defined for the current style lang
(i.e. lang="scss"
).
Now back to the code I replaced: it used to get the content of each global resource with fs.readFileSync
, join them with a line break and return it to be injected as a string in the <style>
block. This is the reason why the bug I fixed was happening because it makes the relative imports relative to the .vue
file instead of relative to the global resource. For instance, if we have styles/global.scss as global resource in the Jest config:
styles/global.scss
@import './fonts.scss';
$font-super-large: $font-size-large * 2;
components/Something.vue
<style lang="scss" module>
.something {
font-size: $font-super-large;
}
</style>
It will be compiled to:
@import './fonts.scss';
$font-super-large: $font-size-large * 2;
.something {
font-size: $font-super-large;
}
which is wrong since fonts.scss is within the styles
directory, not components
. I fixed it by injecting an actual import statement instead which will make the processor understand the relative import as it should.
My code change will produce:
@import '/absolute/path/to/styles/global.scss';
.something {
font-size: $font-super-large;
}
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. I'll merge and release this now. Let's keep an eye after people start upgrading to see if anything was negatively impacted by this change.
It looks like the script you added for the tests isn't running on CI 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the script you added for the tests isn't running on CI 🤔
@lmiller1990 It's running on CircleCI, but not on Github actions since the 26.x
branch is not targeted. It would need to be added to node.yml:
By the way, I removed the test-runner I previously added since it was useless: the e2e tests were already running since each of them is a package of the monorepo and their test scripts were called by yarn workspaces. I'm sorry for the confusion.
4222d23
to
8eb2cee
Compare
4b0050a
to
c0a5f3e
Compare
c0a5f3e
to
39c8d49
Compare
@lmiller1990 Any chance we can merge this PR? |
@lmiller1990 Please? |
@pmrotule Yes, sorry. |
@pmrotule pls try it out: https://github.com/vuejs/vue-jest/releases/tag/v26.0.1 We now use 26.x for Jest 26 support. You changes should be on Let me know if you have any problems. |
@lmiller1990 Thanks! I just tested We are currently working on upgrading to Vue 3 so I should be able to create a PR with the same fix for Vue 3 in the next few days. |
Great, thanks. I'll try to be more responsive with merging and releasing new versions. |
Closes #374
Fix importing relative paths inside a stylesheet being made globally available using the option
jest.globals['vue-jest'].resources
:src/styles/settings/global.scss
considering that src/styles/settings/unit.scss exists, it shouldn't throw the following error
Version
My branch is based on the
v4.0.1
tag which is the version I'm using. It doesn't seem like there is av4
branch I could base my PR on.