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] Prevent build from breaking when .scss file is empty #1193

Merged
merged 2 commits into from Apr 24, 2018

Conversation

crhayes
Copy link
Contributor

@crhayes crhayes commented Apr 16, 2018

I was attempting to set up my build with SASS according to the guides. In order to get the build working as quickly as possible I created an empty app.scss file and imported it into my main module. I expected the build to complete, but instead received this error:

image

Not sure how you feel about this solution, but I traced the problem down to /src/Asset.js:70.

if (this.contents && this.mightHaveDependencies()) {
  await this.parseIfNeeded();
  await this.collectDependencies();
}

this.contents is equal to an empty string, therefore this check evaluates falsy and the contents are not parsed. The end result is that this.ast is not set and remains null.

Because of this, /src/SASSAsset.js:51 blows up:

generate() {
  return [
    {
      type: 'css',
      value: this.ast.css.toString() // this.ast is null
      hasDependencies: false
    }
  ];
}

EDIT:

Pushed up the same fix for LESS files.

@gnijuohz
Copy link
Contributor

Just noticed the same with .less files. Do you want to fix that also? I could submit a PR to your repo if you are busy :)

@crhayes
Copy link
Contributor Author

crhayes commented Apr 17, 2018

@gnijuohz Thanks for the heads up, I fixed it!

@codecov-io
Copy link

Codecov Report

Merging #1193 into master will increase coverage by 2.86%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
+ Coverage   85.59%   88.45%   +2.86%     
==========================================
  Files          77       77              
  Lines        4310     4105     -205     
==========================================
- Hits         3689     3631      -58     
+ Misses        621      474     -147
Impacted Files Coverage Δ
src/assets/LESSAsset.js 100% <ø> (+2.12%) ⬆️
src/assets/SASSAsset.js 100% <ø> (+6.38%) ⬆️
src/assets/ReasonAsset.js 92.3% <0%> (-7.7%) ⬇️
src/transforms/babel.js 88.55% <0%> (-4.98%) ⬇️
src/assets/GLSLAsset.js 93.1% <0%> (-3.2%) ⬇️
src/utils/config.js 86.95% <0%> (-1.45%) ⬇️
src/utils/syncPromise.js 100% <0%> (ø) ⬆️
src/assets/YAMLAsset.js 100% <0%> (ø) ⬆️
src/assets/TOMLAsset.js 100% <0%> (ø) ⬆️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfc1d3c...a197b41. Read the comment docs.

@gnijuohz
Copy link
Contributor

@crhayes great! I was about to submit a PR to your repo haha. Thanks!

@@ -33,7 +33,7 @@ class LESSAsset extends Asset {
return [
{
type: 'css',
value: this.ast.css,
value: this.ast ? this.ast.css : '',
Copy link
Member

Choose a reason for hiding this comment

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

You could fallback to this.contents instead of an empty string, it would end up being the same though. Might be faster as you have it right now.
Would only be really useful if ast can ever be undefined/null while content is not.

@devongovett devongovett merged commit 7a2ba16 into parcel-bundler:master Apr 24, 2018
@crhayes crhayes deleted the fix/scss-empty branch October 4, 2018 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants