feat: add options validation by michael-ciniawsky · Pull Request #224 · webpack/style-loader

michael-ciniawsky · 2017-05-01T02:07:47Z

#What kind of change does this PR introduce?

Style
Feature
Refactor

Did you add tests for your changes?

😛 Yep

If relevant, did you update the README?

👌 #222

Summary

Fixes #223

Add options validation
Initial repo restructure
Initial code style update
Update dependencies (css-loader, file-loader, jsdom)

Does this PR introduce a breaking change?

Nope

bebraw

Can you split this into two PRs? Restructuring and schema. Now it's a little tricky to review. :)

michael-ciniawsky · 2017-05-01T08:59:24Z

@bebraw Yeah sry 😛 it come over me, could we make an exception this time please? 🙃 🙏

michael-ciniawsky · 2017-05-01T09:05:04Z

It's just addStyles && addStylesUrl && urls (fixUrls) => lib, delete fixtures (dead code), add spaces and shorten names in addStyles && addStylesUrl + schema in index, url, useable

michael-ciniawsky · 2017-05-01T09:08:01Z

no logic related changes and the tests pass.. :)

alexander-akait

Seems very good, some note

alexander-akait · 2017-05-01T13:33:28Z

+
+	// Force single-tag solution on IE6-9, which has a hard limit on the # of <style>
+	// tags it will allow on a page
+	if (typeof options.singleton === "undefined") options.singleton = isOldIE();


Why typeof ?

¯_(ツ)_/¯ Good catch, will update :)

alexander-akait · 2017-05-01T13:34:13Z

+		if(domStyle) {
+			domStyle.refs++;
+
+			for(var j = 0; j < domStyle.parts.length; j++) {


Seems it can do in one loop

https://github.com/webpack-contrib/style-loader/pull/224/files/75e1dd4c98e3b1b56e153dec530c965c72ff1fb6

👍 I wll take a look into that

for(var j = 0; j < domStyle.parts.length && item.parts.length; j++) { domStyle.parts[j](item.parts[j]); domStyle.parts.push(addStyle(item.parts[j], options)); }

@evilebottnawi &&or || ? 🙃 the tests pass with &&, but I'm not 💯 on this :D

alexander-akait · 2017-05-01T13:36:20Z

-  "author": "Tobias Koppers @sokra",
  "description": "style loader module for webpack",
+  "engines": {
+    "node": ">= 0.12.0"


Why? 😄 maybe 4 minimum?

Yep but this PR is minor and current style-loader is node =< 4 aswell, I bump it with the webpack-defaults upgrade 😛

alexander-akait · 2017-05-01T13:37:29Z

+    "node": ">= 0.12.0"
+  },
+  "main": "index.js",
+  "files": [


We have not forgotten anything? Maybe be good add to default smoke test?

I this a rhetoric question just to double check or did I miss something obvious ? 🙃

We have && done 😛

alexander-akait

Seems good, I hope we did not break anything 😛

michael-ciniawsky · 2017-05-02T10:06:27Z

@evilebottnawi blocked by webpack/schema-utils#9 it's an enterily stupid issue, but any ideas welcome 😛

michael-ciniawsky · 2017-05-10T13:45:04Z

My apologizes for the PR size again 🙏 , wouldn't happen again 😛

bebraw · 2017-05-10T13:56:30Z

+module.exports = function () {};
+
+module.exports.pitch = function (request) {
+	if (this.cacheable) this.cacheable();


this.cacheable shouldn't be needed in webpack 2 anymore if I understood right.

I left it because it's semver minor & planned to remove with webpack-defaults upgrade in the upcoming major, but we can remove it if consensus is found about it

Ok, that's fine.

bebraw · 2017-05-10T13:56:58Z

+module.exports.pitch = function (request) {
+	if (this.cacheable) this.cacheable();
+
+	var options = loaderUtils.getOptions(this) || {};


Maybe getOptions should return {} by default? Worth checking.

You mean var options = loaderUtils.getOptions(this) returns {} anyways ? mom I will check that :)

Check for sure. If it doesn't, figure out why it doesn't return an {}. That would seem like a sensible default or else we end up with the check everywhere.

use: [ 'style-loader' ] // options === null

getOptions returns null 😕

Ok, maybe escalate to loader-utils. It would make sense to change the default from consumer point of view. I wonder what's the reasoning for null. It's easier if you get an object always.

Yep {} would be better, I open an issue at loader-utils is this blocking the PR ?

Not blocking. The current implementation is fine.

webpack/loader-utils#84

bebraw · 2017-05-10T13:59:06Z

+    },
+    "convertToAbsoluteUrls": {
+      "type": "boolean"
+    }


It would be a good idea to add a description field for each.

For some reason it doesn't get displayed in the OptionsValidationError @schema-utils, but agreed this is mandatory to some point, but I need to fix it in schema-utils first 😛

michael-ciniawsky · 2017-05-22T00:34:27Z

I took a note about #224 (comment) in case it should cause an unforeseen regression and needs to be reverted, works fine locally so far and test pass, but... 😛

michael-ciniawsky added priority: 2 (urgent) semver: Minor pr_action: Review type: Feature labels May 1, 2017

michael-ciniawsky added this to the 0.17.0 milestone May 1, 2017

michael-ciniawsky self-assigned this May 1, 2017

michael-ciniawsky requested review from alexander-akait, bebraw and joshwiens May 1, 2017 02:10

bebraw reviewed May 1, 2017

View reviewed changes

alexander-akait reviewed May 1, 2017

View reviewed changes

michael-ciniawsky added the status: Blocked label May 2, 2017

alexander-akait approved these changes May 2, 2017

View reviewed changes

feat: add options validation

42a87d6

michael-ciniawsky added status: Pending and removed priority: 2 (urgent) status: Blocked labels May 10, 2017

michael-ciniawsky requested review from alexander-akait and bebraw May 10, 2017 13:43

bebraw reviewed May 10, 2017

View reviewed changes

michael-ciniawsky added pr_action: Merge ready and removed pr_action: Review status: Pending labels May 15, 2017

michael-ciniawsky merged commit 4b6b70d into webpack:master May 22, 2017

michael-ciniawsky deleted the schema branch May 22, 2017 00:34

michael-ciniawsky removed the pr_action: Merge ready label May 22, 2017

michael-ciniawsky modified the milestone: 0.19.0 May 22, 2017

michael-ciniawsky mentioned this pull request May 22, 2017

fix(addStyles): revert merged loops #236

Merged

renovate Bot mentioned this pull request Mar 22, 2018

Update style-loader to ^0.21.0 sbardian/DebtTrackerAPI#30

Merged

Uh oh!

Conversation

michael-ciniawsky commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bebraw left a comment

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented May 1, 2017

Uh oh!

michael-ciniawsky commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented May 2, 2017

Uh oh!

michael-ciniawsky commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

michael-ciniawsky commented May 1, 2017 •

edited

Loading

michael-ciniawsky commented May 1, 2017 •

edited

Loading

michael-ciniawsky commented May 1, 2017 •

edited

Loading

michael-ciniawsky May 2, 2017 •

edited

Loading

michael-ciniawsky commented May 10, 2017 •

edited

Loading

michael-ciniawsky May 10, 2017 •

edited

Loading

michael-ciniawsky commented May 22, 2017 •

edited

Loading