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(index): runaway promise #269

Merged
merged 1 commit into from
Oct 10, 2017

Conversation

mcjfunk
Copy link
Contributor

@mcjfunk mcjfunk commented Jun 29, 2017

I'm getting a runaway promise warning:

Warning: a promise was created in a handler at /Users/jmclean/projects/node-graphql-demo/node_modules/postcss-loader/lib/index.js:173:16 but was not returned from it, see http://goo.gl/rRqMUw
    at new Promise (/Users/jmclean/projects/node-graphql-demo/node_modules/bluebird/js/release/promise.js:77:14)

Seems that the loader returns undefined in certain situations, falling back to null fixes it and seems to be the recommended practice from the Bluebird docs: http://goo.gl/rRqMUw

There was a similar fix, about a year ago. #95

@michael-ciniawsky michael-ciniawsky changed the title Fix for runaway promise fix(index): runaway promise Jun 29, 2017
@michael-ciniawsky michael-ciniawsky added this to the 2.0.7 milestone Jun 29, 2017
@michael-ciniawsky
Copy link
Member

@mcjfunk I which usecases does this occur? It might not throw, but looks odd tbh :). e.g the current file would be lost then right?

@mcjfunk
Copy link
Contributor Author

mcjfunk commented Jun 29, 2017

Using the configuration below running the webpack-dev-middleware with a hapi.js server.

{
    test: /\.less$/,
    use: [
        {loader: 'css-loader', options: {importLoaders: 1}},
        {loader: 'postcss-loader', options: {plugins: [autoprefixer({browsers}), cssnano({safe: true})]}},
        'less-loader'
    ]
},

There is no promise warning outside of the 'webpack-dev-middleware` workflow.

With (and without) the fix, the styles are correctly processed and injected onto the page. The fix quiets down the promise warning that happens on every less file it processes.

Looks like others have had to implement a similar fix:
vuejs/vue-loader#625

Thanks!

@alexander-akait
Copy link
Member

@mcjfunk seems related to less-loader, can your create minimum reproducible test repo?

@mcjfunk
Copy link
Contributor Author

mcjfunk commented Jun 29, 2017

I won't have a chance to put together a test repo anytime soon :(.

I tried a test with a CSS file instead of less, and removed the less-loader. The warnings were still present.

When I remove the css-loader, the warnings go away.

@michael-ciniawsky
Copy link
Member

I need a reproducible case for this to land it in postcss-loader directly, as long as it isn't coming from the loader itself, I would like to avoid adding workarounds. css-loader will get an overhaul soon. It is dog slow and buggy atm. If you tracked it to css-loader open an issue there aswell please either @evilebottnawi or I can fix it there also

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Just a note for now, in case we need it

@@ -165,7 +165,7 @@ module.exports = function loader (css, map) {
* @param {String} result Result (JS Module)
* @param {Object} map Source Map
*/
return cb(null, `module.exports = ${JSON.stringify(css)}`, map)
return cb(null, `module.exports = ${JSON.stringify(css)}`, map) || null
Copy link
Member

Choose a reason for hiding this comment

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

cb(null, `module.exports = ${JSON.stringify(css)}`, map)
return null

@@ -175,7 +175,7 @@ module.exports = function loader (css, map) {
* @param {String} css Result (Raw Module)
* @param {Object} map Source Map
*/
return cb(null, css, map)
return cb(null, css, map) || null
Copy link
Member

Choose a reason for hiding this comment

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

cb(null, css, map)
return null

Copy link

@DmitriWolf DmitriWolf Sep 29, 2017

Choose a reason for hiding this comment

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

I'm working on a React app that spits out a long string of broken promise warnings every time I build, and either of these two fixes makes them all go away.
Seems like a trivial fix to a constantly annoying problem.
Please merge one or the other. Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

@DmitriWolf Are you also using bluebird ?

Choose a reason for hiding this comment

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

We are!

Copy link

@DmitriWolf DmitriWolf left a comment

Choose a reason for hiding this comment

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

Simple fix, looks good!

@@ -175,7 +175,7 @@ module.exports = function loader (css, map) {
* @param {String} css Result (Raw Module)
* @param {Object} map Source Map
*/
return cb(null, css, map)
return cb(null, css, map) || null
Copy link

@DmitriWolf DmitriWolf Sep 29, 2017

Choose a reason for hiding this comment

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

I'm working on a React app that spits out a long string of broken promise warnings every time I build, and either of these two fixes makes them all go away.
Seems like a trivial fix to a constantly annoying problem.
Please merge one or the other. Thank you.

@timmolendijk
Copy link

Returning null there instead of undefined is definitely the way to go!

Previous versions (1.3.3 f.e.) returned null there, so let's stick to that behavior.

@michael-ciniawsky
Copy link
Member

Released in v2.0.7

@michael-ciniawsky michael-ciniawsky removed this from the 2.0.8 milestone Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants