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: stricter @import tolerance #593

Merged
merged 4 commits into from
Aug 17, 2017
Merged

fix: stricter @import tolerance #593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b68e75
Added failed test for @import-normalize
swernerx Aug 16, 2017
3b25b93
Fixed import handling not matching import like constructs.
swernerx Aug 16, 2017
3087559
Made tolerant for null match on urls inside import
swernerx Aug 16, 2017
be3eef5
Added request test case
swernerx Aug 16, 2017
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
6 changes: 3 additions & 3 deletions lib/processCss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ var parserPlugin = postcss.plugin("css-loader-parser", function(options) {
}

if(options.import) {
css.walkAtRules(/import/i, function(rule) {
css.walkAtRules(/^import$/i, function(rule) {
Copy link
Member

Choose a reason for hiding this comment

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

/^@import$/i

Copy link
Member

@alexander-akait alexander-akait Aug 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@michael-ciniawsky walkAtRules already walks only on @something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The at-sign is not needed here, right.

var values = Tokenizer.parseValues(rule.params);
var url = values.nodes[0].nodes[0];
if(url.type === "url") {
if(url && url.type === "url") {
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to get the same error handling about malformed URLs for non-existing URLs. This was actually the original error, where I got something like can't look up "type" of undefined.

Copy link
Member

Choose a reason for hiding this comment

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

@swernerx can your add tests for this, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be a new kind of test which tests for throwing errors. We do not do this currently in CSS loader it seems... at least not in the importTest.js file.

Copy link
Member

Choose a reason for hiding this comment

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

@swernerx we should add tests or avoid this fix here (move to separate issue or PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

@swernerx oh no 😄 your test was right with @import, we should tests about url && url.type === "url"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I did. Without the new handling it should just throw some hard error.

url = url.url;
} else if(url.type === "string") {
} else if(url && url.type === "string") {
Copy link
Member

Choose a reason for hiding this comment

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

?

url = url.value;
} else throw rule.error("Unexpected format " + rule.params);
if (!url.replace(/\s/g, '').length) {
Expand Down
20 changes: 18 additions & 2 deletions test/importTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/*globals describe */

var test = require("./helpers").test;
var assert = require('assert');

var helpers = require("./helpers");
var test = helpers.test;
var testError = helpers.testError;

describe("import", function() {
test("import", "@import url(test.css);\n.class { a: b c d; }", [
Expand Down Expand Up @@ -66,4 +70,16 @@ describe("import", function() {
test("import disabled", "@import url(test.css);\n.class { a: b c d; }", [
[1, "@import url(test.css);\n.class { a: b c d; }", ""]
], "?-import");
});
test("@import-normalize left untouched", "@import-normalize;", [
[1, "@import-normalize;", ""]
]);
testError("@import without url", "@import;", function(err) {
assert.equal(err.message, [
'Unexpected format (1:1)',
'',
'> 1 | @import;',
' | ^',
'',
].join('\n'))
})
})