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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 28, 2021
Merged

fix: allow importing relative paths in global resources #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
473b5fe
fix: allow importing relative paths in global resources
pmrotule Oct 11, 2021
39c8d49
fix: allow importing relative paths in style blocks
pmrotule Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions e2e/2.x/style/colors.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@primary-color: "red";
1 change: 1 addition & 0 deletions e2e/2.x/style/colors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
$primary-color: #333;
4 changes: 3 additions & 1 deletion e2e/2.x/style/variables.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
@primary-color: "red";
@import "./colors.less";

@font-size: 16px;
4 changes: 3 additions & 1 deletion e2e/2.x/style/variables.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
$primary-color: #333;
@import './colors.scss';

$font-size: 16px;
19 changes: 14 additions & 5 deletions packages/vue2-jest/lib/process-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const path = require('path')
const fs = require('fs')
const cssExtract = require('extract-from-css')
const getVueJestConfig = require('./utils').getVueJestConfig
const compileStyle = require('@vue/component-compiler-utils').compileStyle
Expand All @@ -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))
Copy link
Member

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?

Copy link
Contributor Author

@pmrotule pmrotule Oct 12, 2021 •
edited
Loading

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?

Copy link
Member

@lmiller1990 lmiller1990 Oct 25, 2021 •
edited
Loading

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 🤔

Copy link
Contributor Author

@pmrotule pmrotule Oct 25, 2021 •
edited
Loading

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:

image

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.

.map(resourcePath => fs.readFileSync(resourcePath).toString())
.join('\n')
.map(resource => {
const absolutePath = path.resolve(process.cwd(), resource)
return `${getImportLine(lang, absolutePath)}\n`
})
.join('')
}
return globalResources
}

function getImportLine(lang, filePath) {
const importLines = {
default: `@import "${filePath}";`,
sass: `@import "${filePath}"`
}
return importLines[lang] || importLines.default
}

function extractClassMap(cssCode) {
const cssNames = cssExtract.extractClasses(cssCode)
const cssMap = {}
Expand All @@ -32,6 +40,7 @@ function extractClassMap(cssCode) {
function getPreprocessOptions(lang, filePath, jestConfig) {
if (lang === 'scss' || lang === 'sass') {
return {
filename: filePath,
Copy link
Contributor Author

@pmrotule pmrotule Oct 11, 2021 •
edited
Loading

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.

importer: (url, prev, done) => ({
file: applyModuleNameMapper(
url,
Expand Down