Skip to content

fix: provide an unquoted path argument for opt.resolve(path)#127

Merged
madyankin merged 1 commit intomadyankin:masterfrom
kamilic:provide-an-unquoted-path-argument
Jul 23, 2021
Merged

fix: provide an unquoted path argument for opt.resolve(path)#127
madyankin merged 1 commit intomadyankin:masterfrom
kamilic:provide-an-unquoted-path-argument

Conversation

@kamilic
Copy link
Copy Markdown
Contributor

@kamilic kamilic commented Jul 22, 2021

Argument path is a quoted string In opt.resolve(path) which is not convient for user to use this api.
Also, it is not an expected behaviour for pr #126.

For example:

.a {
  compose: b from "@/scss/compose.css";
}

Current behaviour:

postcssModules({
  resolve(path) {
     console.log(path) // "\"@/scss/compose.css\""
  }
})

Expected behaviour:

postcssModules({
  resolve(path) {
     console.log(path) // "@/scss/compose.css"
  }
})

Sorry for my mistake.

@kamilic kamilic force-pushed the provide-an-unquoted-path-argument branch 2 times, most recently from 36bc6ff to 5de1d03 Compare July 22, 2021 06:41
madyankin
madyankin requested changes Jul 23, 2021
View reviewed changes
@kamilic kamilic force-pushed the provide-an-unquoted-path-argument branch from 5de1d03 to ebc8218 Compare July 23, 2021 10:29
@kamilic
Copy link
Copy Markdown
Contributor Author

kamilic commented Jul 23, 2021 •
edited
Loading

@madyankin done.

madyankin
madyankin reviewed Jul 23, 2021
View reviewed changes
src/unquote/index.js
@@ -0,0 +1,16 @@
// copied from https://github.com/lakenen/node-unquote

var reg = /['"]/;
Copy link
Copy Markdown
Owner

@madyankin madyankin Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this regexp will replace only the quotes and leave the backslashes. Shouldn't it be [\'\"]?

Copy link
Copy Markdown
Contributor Author

@kamilic kamilic Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint tells me it can be removed.

Copy link
Copy Markdown
Contributor Author

@kamilic kamilic Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Copy Markdown
Owner

@madyankin madyankin Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I have read the code not thorough enough the first time. Everything is ok

Copy link
Copy Markdown
Contributor Author

@kamilic kamilic Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madyankin If you want to keep it like where copied from, I will change it to [\'\"] and add a // eslint-disable no-useless-escape line to disable this rule.

Copy link
Copy Markdown
Contributor Author

@kamilic kamilic Jul 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madyankin ok.

@madyankin madyankin merged commit b765440 into madyankin:master Jul 23, 2021
This was referenced Jul 23, 2021
Merged
Closed
Closed
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@madyankin madyankin madyankin requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kamilic @madyankin @kamilic-work