-
Notifications
You must be signed in to change notification settings - Fork 27.6k
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 preprocessor loader error #10235
Conversation
I can open a separate issue for this if requested, but I encountered it here while making the new test. These assertionless tests for successful build don't seem to actually fail on compile errors. I worked around it by checking the stderr to make sure the regression isn't present, but was wondering if these tests should do something further like check the build directory or similarly check for errors in
Update: looks like the same discrepancy was discovered on your end, updated the new test to resemble #10237 |
Stats from current PRDefault Server Mode (Decrease detected ✓)General Overall decrease ✓
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Rendered Page Sizes
Serverless Mode (Decrease detected ✓)General Overall decrease ✓
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Serverless bundles
Commit: e2f3159 |
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.
This doesn't seem correct.
Per, https://github.com/bholloway/resolve-url-loader/blob/master/packages/resolve-url-loader/README.md, the intended order is:
rules: [
{
test: /\.scss$/,
use: [
...
{
loader: 'css-loader',
options: {...}
}, {
loader: 'resolve-url-loader',
options: {...}
}, {
loader: 'sass-loader',
options: {
sourceMap: true,
sourceMapContents: false
}
}
]
},
...
]
This was the issue and fix I found elsewhere. I'll look into this further now that I have a test to experiment against but hopefully the tests will help us be sure it's working in whichever order. For reference the error is:
|
I've figured out the steps to create the error and produced an example repo here: https://github.com/alejalapeno/next-sass-loader-order I replicated the example from Requirements:
The removal of the preprocessor reversal is the only thing I've found to solve it. Removal of the reversal still seems to resolve both module + global urls. Interestingly enough, removal of the |
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.
Found our bug:
> let a = [1,2,3]
undefined
> a
[ 1, 2, 3 ]
> a.reverse()
[ 3, 2, 1 ]
> a
[ 3, 2, 1 ]
edit: fixed in 254a661
Stats from current PRDefault Server ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Rendered Page Sizes
Serverless ModeGeneral
Client Bundles (main, webpack, commons)
Client Bundles (main, webpack, commons) Modern
Legacy Client Bundles (polyfills)
Client Pages
Client Pages Modern
Client Build Manifests
Serverless bundles
Commit: 3352023 |
Stats from current PRDefault Server ModeGeneral Overall increase
|
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
buildDuration | 11.3s | 11s | -318ms |
nodeModulesSize | 52.1 MB | 52.1 MB |
Client Bundles (main, webpack, commons)
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
main-HASH.js gzip | 5.1 kB | 5.1 kB | ✓ |
webpack-HASH.js gzip | 746 B | 746 B | ✓ |
4952ddcd88e7..54d3.js gzip | 4.68 kB | 4.68 kB | ✓ |
commons.HASH.js gzip | 4.06 kB | 4.06 kB | ✓ |
de003c3a9d30..d6ae.js gzip | 16.2 kB | 16.2 kB | ✓ |
framework.HASH.js gzip | 39.1 kB | 39.1 kB | ✓ |
Overall change | 69.9 kB | 69.9 kB | ✓ |
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
main-HASH.module.js gzip | 4.1 kB | 4.1 kB | ✓ |
webpack-HASH..dule.js gzip | 746 B | 746 B | ✓ |
4952ddcd88e7..dule.js gzip | 5.56 kB | 5.56 kB | ✓ |
de003c3a9d30..dule.js gzip | 15.1 kB | 15.1 kB | ✓ |
framework.HA..dule.js gzip | 39.1 kB | 39.1 kB | ✓ |
Overall change | 64.6 kB | 64.6 kB | ✓ |
Legacy Client Bundles (polyfills)
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 4.76 kB | 4.76 kB | ✓ |
Overall change | 4.76 kB | 4.76 kB | ✓ |
Client Pages
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
_app.js gzip | 1.15 kB | 1.15 kB | ✓ |
_error.js gzip | 4.07 kB | 4.07 kB | ✓ |
hooks.js gzip | 779 B | 779 B | ✓ |
index.js gzip | 222 B | 222 B | ✓ |
link.js gzip | 2.89 kB | 2.89 kB | ✓ |
routerDirect.js gzip | 283 B | 283 B | ✓ |
withRouter.js gzip | 282 B | 282 B | ✓ |
Overall change | 9.68 kB | 9.68 kB | ✓ |
Client Pages Modern
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
_app.module.js gzip | 576 B | 576 B | ✓ |
_error.module.js gzip | 3.06 kB | 3.06 kB | ✓ |
hooks.module.js gzip | 371 B | 371 B | ✓ |
index.module.js gzip | 212 B | 212 B | ✓ |
link.module.js gzip | 2.46 kB | 2.46 kB | ✓ |
routerDirect..dule.js gzip | 273 B | 273 B | ✓ |
withRouter.m..dule.js gzip | 272 B | 272 B | ✓ |
Overall change | 7.22 kB | 7.22 kB | ✓ |
Client Build Manifests
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
_buildManifest.js gzip | 61 B | 61 B | ✓ |
_buildManife..dule.js gzip | 61 B | 61 B | ✓ |
Overall change | 122 B | 122 B | ✓ |
Rendered Page Sizes
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
index.html gzip | 1.02 kB | 1.02 kB | ✓ |
link.html gzip | 1.03 kB | 1.03 kB | ✓ |
withRouter.html gzip | 1.02 kB | 1.02 kB | ✓ |
Overall change | 3.07 kB | 3.07 kB | ✓ |
Serverless Mode
General Overall increase ⚠️
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
buildDuration | 11.9s | 11.9s | -11ms |
nodeModulesSize | 52.1 MB | 52.1 MB |
Client Bundles (main, webpack, commons)
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
main-HASH.js gzip | 5.1 kB | 5.1 kB | ✓ |
webpack-HASH.js gzip | 746 B | 746 B | ✓ |
4952ddcd88e7..54d3.js gzip | 4.68 kB | 4.68 kB | ✓ |
commons.HASH.js gzip | 4.06 kB | 4.06 kB | ✓ |
de003c3a9d30..d6ae.js gzip | 16.2 kB | 16.2 kB | ✓ |
framework.HASH.js gzip | 39.1 kB | 39.1 kB | ✓ |
Overall change | 69.9 kB | 69.9 kB | ✓ |
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
main-HASH.module.js gzip | 4.1 kB | 4.1 kB | ✓ |
webpack-HASH..dule.js gzip | 746 B | 746 B | ✓ |
4952ddcd88e7..dule.js gzip | 5.56 kB | 5.56 kB | ✓ |
de003c3a9d30..dule.js gzip | 15.1 kB | 15.1 kB | ✓ |
framework.HA..dule.js gzip | 39.1 kB | 39.1 kB | ✓ |
Overall change | 64.6 kB | 64.6 kB | ✓ |
Legacy Client Bundles (polyfills)
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 4.76 kB | 4.76 kB | ✓ |
Overall change | 4.76 kB | 4.76 kB | ✓ |
Client Pages
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
_app.js gzip | 1.15 kB | 1.15 kB | ✓ |
_error.js gzip | 4.07 kB | 4.07 kB | ✓ |
hooks.js gzip | 779 B | 779 B | ✓ |
index.js gzip | 222 B | 222 B | ✓ |
link.js gzip | 2.89 kB | 2.89 kB | ✓ |
routerDirect.js gzip | 283 B | 283 B | ✓ |
withRouter.js gzip | 282 B | 282 B | ✓ |
Overall change | 9.68 kB | 9.68 kB | ✓ |
Client Pages Modern
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
_app.module.js gzip | 576 B | 576 B | ✓ |
_error.module.js gzip | 3.06 kB | 3.06 kB | ✓ |
hooks.module.js gzip | 371 B | 371 B | ✓ |
index.module.js gzip | 212 B | 212 B | ✓ |
link.module.js gzip | 2.46 kB | 2.46 kB | ✓ |
routerDirect..dule.js gzip | 273 B | 273 B | ✓ |
withRouter.m..dule.js gzip | 272 B | 272 B | ✓ |
Overall change | 7.22 kB | 7.22 kB | ✓ |
Client Build Manifests
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
_buildManifest.js gzip | 61 B | 61 B | ✓ |
_buildManife..dule.js gzip | 61 B | 61 B | ✓ |
Overall change | 122 B | 122 B | ✓ |
Serverless bundles
zeit/next.js canary | alejalapeno/next.js sass-fix | Change | |
---|---|---|---|
_error.js gzip | 46.6 kB | 46.6 kB | ✓ |
hooks.html gzip | 1.06 kB | 1.06 kB | ✓ |
index.js gzip | 46.8 kB | 46.8 kB | ✓ |
link.js gzip | 72.4 kB | 72.4 kB | ✓ |
routerDirect.js gzip | 70.4 kB | 70.4 kB | ✓ |
withRouter.js gzip | 70.5 kB | 70.5 kB | ✓ |
Overall change | 308 kB | 308 kB | ✓ |
Commit: 254a661
@Timer Ahhh, because of the reversal in two places and mutating the original the second (global?) was essentially undoing the first reverse? Great catch! |
Running resolve-url-loader before(?) sass-loader results in errors. Easiest to reproduce with a double slash comment
// Comment
in a.scss
file.Added test to prevent regression.