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.

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

Merged
merged 7 commits into from Jan 28, 2020
Merged

Fix preprocessor loader error #10235

merged 7 commits into from Jan 28, 2020

Conversation

alejalapeno
Copy link
Contributor

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.

@alejalapeno
Copy link
Contributor Author

alejalapeno commented Jan 23, 2020

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 stderr?

it('should build successfully', async () => {
  await nextBuild(appDir)
})

Update: looks like the same discrepancy was discovered on your end, updated the new test to resemble #10237

@ijjk
Copy link
Member

ijjk commented Jan 23, 2020

Stats from current PR

Default Server Mode (Decrease detected ✓)
General Overall decrease ✓
zeit/next.js canary alejalapeno/next.js sass-fix Change
buildDuration 12.5s 12.5s -18ms
nodeModulesSize 51.3 MB 51.3 MB -232 B
Client Bundles (main, webpack, commons)
zeit/next.js canary alejalapeno/next.js sass-fix Change
main-HASH.js gzip 5.11 kB 5.11 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..d57b.js gzip 16.1 kB 16.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 69.8 kB 69.8 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 kB 15 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 64.5 kB 64.5 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 (Decrease detected ✓)
General Overall decrease ✓
zeit/next.js canary alejalapeno/next.js sass-fix Change
buildDuration 13.1s 13s -124ms
nodeModulesSize 51.3 MB 51.3 MB -232 B
Client Bundles (main, webpack, commons)
zeit/next.js canary alejalapeno/next.js sass-fix Change
main-HASH.js gzip 5.11 kB 5.11 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..d57b.js gzip 16.1 kB 16.1 kB
framework.HASH.js gzip 39.1 kB 39.1 kB
Overall change 69.8 kB 69.8 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 kB 15 kB
framework.HA..dule.js gzip 39.1 kB 39.1 kB
Overall change 64.5 kB 64.5 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.2 kB 46.2 kB
hooks.html gzip 1.06 kB 1.06 kB
index.js gzip 46.4 kB 46.4 kB
link.js gzip 71.9 kB 71.9 kB
routerDirect.js gzip 69.9 kB 69.9 kB
withRouter.js gzip 69.9 kB 69.9 kB
Overall change 305 kB 305 kB

Commit: e2f3159

Copy link
Member

@Timer Timer left a 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
        }
      }
    ]
  },
  ...
]

@alejalapeno
Copy link
Contributor Author

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:

Failed to compile.·

./styles/global.scss
Error: resolve-url-loader: CSS error
  file:///next.js/test/integration/scss-fixtures/loader-order/styles/global.scss:1:4: Unknown word

@alejalapeno
Copy link
Contributor Author

alejalapeno commented Jan 24, 2020

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 resolve-url-loader docs (with file name changes to match Next's structure.)

Requirements:

  • Applies only to global Sass. Does not affect modules.
  • Sass file must have Sass features. Seems to be skipped over by the sass-loader otherwise.
  • Loader then fails on several features, including (crucially) not being able to resolve URL's. The fail on double-slash comments is just an easy way to test for the same issue.

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 reverse() for modules loaders in modules.ts has the opposite effect and breaks them in the same manner...

Copy link
Member

@Timer Timer left a 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

@ijjk
Copy link
Member

ijjk commented Jan 28, 2020

Stats from current PR

Default Server Mode
General
zeit/next.js canary alejalapeno/next.js sass-fix Change
buildDuration 10.8s 10.9s ⚠️ +148ms
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
zeit/next.js canary alejalapeno/next.js sass-fix Change
buildDuration 11.6s 11.4s -118ms
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: 3352023

@Timer Timer added this to the 9.2.2 milestone Jan 28, 2020
@ijjk
Copy link
Member

ijjk commented Jan 28, 2020

Stats from current PR

Default Server Mode
General Overall increase ⚠️
zeit/next.js canary alejalapeno/next.js sass-fix Change
buildDuration 11.3s 11s -318ms
nodeModulesSize 52.1 MB 52.1 MB ⚠️ +104 B
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 ⚠️ +104 B
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

@alejalapeno
Copy link
Contributor Author

@Timer Ahhh, because of the reversal in two places and mutating the original the second (global?) was essentially undoing the first reverse? Great catch!

@vercel vercel locked as resolved and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants