Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

refactor: don't throw errors in case of child context #390

Closed
wants to merge 1 commit into from

Conversation

digitalkaoz
Copy link

@digitalkaoz digitalkaoz commented Feb 2, 2017

will work with this conditional webpack rule (in conjunction with html-webpack-loader):

            {
                test: /\.scss$/,
                oneOf: [
                    {test: /html-webpack-plugin/, use: "null-loader"},
                    {
                        use: ExtractTextPlugin.extract({
                            fallbackLoader: 'style-loader',
                            loader: ['css-loader', 'sass-loader']
                        })
                    }
                ]
            },

will work with this conditional webpack rule:

```js
            {
                test: /\.scss$/,
                oneOf: [
                    {test: /html-webpack-plugin/, use: "null-loader"},
                    {
                        use: ExtractTextPlugin.extract({
                            fallbackLoader: 'style-loader',
                            loader: ['css-loader', 'sass-loader']
                        })
                    }
                ]
            },
```
@codecov-io
Copy link

codecov-io commented Feb 2, 2017

Codecov Report

Merging #390 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
+ Coverage   87.06%   87.28%   +0.21%     
==========================================
  Files           5        5              
  Lines         348      346       -2     
  Branches       71       70       -1     
==========================================
- Hits          303      302       -1     
+ Misses         45       44       -1
Impacted Files Coverage Δ
loader.js 90.24% <100%> (ø)
...rib/extract-text-webpack-plugin/ExtractedModule.js
...pack-contrib/extract-text-webpack-plugin/loader.js
...bpack-contrib/extract-text-webpack-plugin/index.js
...extract-text-webpack-plugin/OrderUndefinedError.js
...ib/extract-text-webpack-plugin/schema/validator.js
ExtractedModule.js 89.18% <ø> (ø)
schema/validator.js 100% <ø> (ø)
OrderUndefinedError.js 37.5% <ø> (ø)
... and 1 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 e8e9780...5b082b0. Read the comment docs.

@digitalkaoz
Copy link
Author

i signed to CLA but it wont show up here...

@bebraw
Copy link
Contributor

bebraw commented Feb 2, 2017

Yeah, the CLA checker is currently disabled. No need to worry about that.

I think we have to wait on @sokra on whether this is a good idea or not. It would fix your use case, but make it easier to miss the connection.

@digitalkaoz
Copy link
Author

yeah i know...but the line below returned an empty string without throwing in case of a child context...so i thought i would be ok to fail silent

@bebraw
Copy link
Contributor

bebraw commented Feb 2, 2017

Maybe a warning would be a softer approach. At worst case we'll add a flag for disabling the error. But it's Tobias' call to make.

@michael-ciniawsky michael-ciniawsky changed the title dont throw errors in case of child-context refactor: don't throw errors in case of child context Apr 22, 2017
@michael-ciniawsky michael-ciniawsky added this to the 2.1.x milestone Apr 22, 2017
@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 25, 2017

@digitalkaoz What is your particular usecase for this? In any case, please close and reopen the PR to trigger CLA Bot again and sign to CLA

We definitely need at least to console.warn('Current Error Message') if this is going to land :)

'refer to https://github.com/webpack/extract-text-webpack-plugin for the usage example'
);
} else if(this[NS] === false) {
if(this[NS] === undefined || this[NS] === false) {
return "";
Copy link
Member

Choose a reason for hiding this comment

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

this.emitWarning(`
  The extract-text-webpack-plugin loader is used without the corresponding plugin,
  in case this is unintentional refer to https://github.com/webpack/extract-text-webpack-plugin 
  for correct usage (example)
`)

Or similiar 😛

Copy link
Member

@joshwiens joshwiens May 2, 2017

Choose a reason for hiding this comment

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

Should probably emit here no? ( Ha, finally got to not be on the receiving end of console.warn hand slap! )

In any case, this needs to do something. The black hole approach is less than optimal for consumers, particularly those lacking a deep understanding of what extract text is doing contextually.

Copy link
Member

Choose a reason for hiding this comment

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

good catch, yep this.emitWarning 😛, example updated ☝️

@Nevraeka
Copy link

Any updates on this?

@michael-ciniawsky
Copy link
Member

@digitalkaoz friendly ping

@michael-ciniawsky
Copy link
Member

We are close the a major release, currently in beta npm i -D extract-text-webpack-plugin and therefore I will close this PR here, feel free to open a new PR with this changes, when #540 landed on master

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants