feat(index): add `data-url` attribute #317

bukuta · 2018-04-20T13:20:24Z

What kind of change does this PR introduce?

Feature

Did you add tests for your changes?
Yes

If relevant, did you update the README?
Yes

Summary

Add the support of dynamic attributes with placeholder as file-loader does, supply some useful informations when debugging.

It may fix the problem

Does this PR introduce a breaking change?

No breaking change

Other information
None

jsf-clabot · 2018-04-20T13:20:25Z

All committers have signed the CLA.

alexander-akait · 2018-04-20T13:22:42Z

index.js

@@ -12,7 +13,16 @@ module.exports = function () {};
 module.exports.pitch = function (request) {
 	if (this.cacheable) this.cacheable();

-	var options = loaderUtils.getOptions(this) || {};
+	var options = clone(loaderUtils.getOptions(this) || {});


Don't use clone, it is decrease perf and bad practice

Ok.
clone was used because the recommend in loader-utils.
Now it is removed, and the logic is splited

alexander-akait · 2018-04-20T13:22:55Z

Also please describe use case. Thanks!

codecov · 2018-04-20T13:23:17Z

Codecov Report

Merging #317 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
+ Coverage   98.43%   98.66%   +0.22%     
==========================================
  Files           4        4              
  Lines          64       75      +11     
  Branches       21       24       +3     
==========================================
+ Hits           63       74      +11     
  Misses          1        1

Impacted Files	Coverage Δ
index.js	`100% <100%> (ø)`	⬆️

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 e4fa68a...974cabd. Read the comment docs.

bukuta · 2018-04-23T02:45:10Z

@evilebottnawi

Some times we need some ways to find out

whether or not the styles processed with the right loaders and injected to dom
the order of the styles

The differences can be distinct.

vs

alexander-akait · 2018-04-23T09:47:34Z

@bukuta Please accept CLA

bukuta · 2018-04-23T10:13:41Z

@evilebottnawi OK

alexander-akait · 2018-04-23T10:14:40Z

package.json

@@ -15,6 +15,7 @@
    "options.json"
  ],
  "dependencies": {
+    "clone": "^1.0.4",


Please remove this

alexander-akait · 2018-04-23T10:15:03Z

index.js

@@ -13,6 +13,16 @@ module.exports.pitch = function (request) {
 	if (this.cacheable) this.cacheable();

 	var options = loaderUtils.getOptions(this) || {};
+	var context = options.context || this.rootContext || (this.options && this.options.context);
+	var attrs = {};
+	for(var key in options.attrs||{}){


space between ||

alexander-akait · 2018-04-23T10:15:32Z

index.js

+	var context = options.context || this.rootContext || (this.options && this.options.context);
+	var attrs = {};
+	for(var key in options.attrs||{}){
+		var name = loaderUtils.interpolateName(this,options.attrs[key],{


Can we add simple regex to detect [here]? To avoid perf problem?

I add a flag and a regexp

alexander-akait · 2018-04-23T11:32:02Z

index.js

@@ -83,6 +96,8 @@ module.exports.pitch = function (request) {
 		"",
 		"var options = " + JSON.stringify(options),
 		"",
+		dynamicAttributesMatched ? "options.attrs = " + JSON.stringify(attrs) : "",


Just check Object.keys(attrs).length > 0

The attrs object contains all attributes plain and dynamic. and ony dynamic attributes detected, the override is need.

alexander-akait · 2018-04-23T12:04:23Z

test/basic.test.js

+    // Run
+    let expected = [
+      existingStyle,
+      `<style x-from="style.css" type="text/css">${requiredCss}</style>`


Can we add tests around nested (i.e. with context option) to ensure path is right

alexander-akait

All good, just add one tests 👍

ooflorent · 2018-04-23T12:18:23Z

README.md

@@ -210,7 +210,7 @@ import style from './file.css'
 {
  test: /\.css$/,
  use: [
-    { loader: 'style-loader', options: { attrs: { id: 'id' } } }
+    { loader: 'style-loader', options: { attrs: { id: 'id' , 'x-from': '[path][name].[ext]'} } }


What about a data attribute instead of a non-standard one?

@bukuta let's use by default data-from in tests and README and we can merge this

alexander-akait · 2018-04-24T09:59:21Z

README.md

@@ -210,7 +210,7 @@ import style from './file.css'
 {
  test: /\.css$/,
  use: [
-    { loader: 'style-loader', options: { attrs: { id: 'id' } } }
+    { loader: 'style-loader', options: { attrs: { id: 'id' , 'x-from': '[path][name].[ext]'} } }


@bukuta let's use by default data-from in tests and README and we can merge this

alexander-akait

Looks very good. Thanks!

michael-ciniawsky · 2018-05-05T17:27:19Z

README.md

@@ -210,7 +210,7 @@ import style from './file.css'
 {
  test: /\.css$/,
  use: [
-    { loader: 'style-loader', options: { attrs: { id: 'id' } } }
+    { loader: 'style-loader', options: { attrs: { id: 'id' , 'data-from': '[path][name].[ext]'} } }


data-from => data-url

michael-ciniawsky · 2018-05-05T17:28:24Z

index.js

@@ -14,6 +14,19 @@ module.exports.pitch = function (request) {

 	var options = loaderUtils.getOptions(this) || {};

+	var attrs = {};


Use the request or this.resourcePath and shorten it instead of interpolating, we should already have the url here

var attrs = { 'data-url': request || this.resourcePath }

alexander-akait

@bukuta can you describe real use case? Why just don't use source maps?

michael-ciniawsky · 2018-08-28T02:33:24Z

Closing due to inactivity feel free to reopen once the requested chances are addressed :)

adriengcql · 2019-07-19T23:42:38Z

Looks like it's almost done, can we reopen?

m-orsh · 2020-01-09T21:26:32Z

Lets say I have 2 standalone apps. Each one needs to check whether the css libraries it needs have been loaded prior to loading them. If I load app 1, and this loads some css libs that app 2 also will use, without a method of identifying the css I have imported, how can I identify if the css has already been loaded? I think the only good way is with this feature.
example:
-app 1 loads bootstrap.css
-app 2 now checks whether it should load the same bootstrap.css
-there is no way of telling bootstrap.css has been loaded since it was loaded in a <style> tag and there is no good way of labelling which style tag contains bootstrap.css

support dynamic attrs

034a6d6

alexander-akait requested changes Apr 20, 2018

View reviewed changes

alexander-akait added pr_action: Review type: Feature semver: Minor pr_action: Discuss priority: 5 (nice to have) labels Apr 20, 2018

remove clone as requirement

9cf79db

alexander-akait requested changes Apr 23, 2018

View reviewed changes

remove unnecessary dependent and fix some bad code

e7783fc

alexander-akait requested changes Apr 23, 2018

View reviewed changes

alexander-akait reviewed Apr 23, 2018

View reviewed changes

alexander-akait requested changes Apr 23, 2018

View reviewed changes

ooflorent reviewed Apr 23, 2018

View reviewed changes

add tests about dynamic attributes [path]

78559f3

alexander-akait requested changes Apr 24, 2018

View reviewed changes

update readme and tests

974cabd

alexander-akait approved these changes Apr 24, 2018

View reviewed changes

ooflorent approved these changes Apr 24, 2018

View reviewed changes

michael-ciniawsky requested changes May 5, 2018

View reviewed changes

michael-ciniawsky changed the title ~~feat: support dynamic attrs~~ feat(index): add data-url attribute May 5, 2018

alexander-akait requested changes May 5, 2018

View reviewed changes

michael-ciniawsky closed this Aug 28, 2018

michael-ciniawsky added status: Declined and removed pr_action: Discuss pr_action: Review priority: 5 (nice to have) labels Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(index): add `data-url` attribute #317

feat(index): add `data-url` attribute #317

bukuta commented Apr 20, 2018

jsf-clabot commented Apr 20, 2018 •

edited

Loading

alexander-akait Apr 20, 2018

bukuta Apr 23, 2018

alexander-akait commented Apr 20, 2018

codecov bot commented Apr 20, 2018 •

edited

Loading

bukuta commented Apr 23, 2018

alexander-akait commented Apr 23, 2018

bukuta commented Apr 23, 2018

alexander-akait Apr 23, 2018

bukuta Apr 23, 2018

alexander-akait Apr 23, 2018 •

edited

Loading

bukuta Apr 23, 2018

alexander-akait Apr 23, 2018

bukuta Apr 23, 2018

alexander-akait Apr 23, 2018

bukuta Apr 23, 2018

alexander-akait Apr 23, 2018

alexander-akait left a comment

ooflorent Apr 23, 2018

alexander-akait Apr 23, 2018

alexander-akait Apr 24, 2018

alexander-akait Apr 24, 2018

alexander-akait left a comment

michael-ciniawsky May 5, 2018

michael-ciniawsky May 5, 2018 •

edited

Loading

alexander-akait left a comment •

edited

Loading

michael-ciniawsky commented Aug 28, 2018

adriengcql commented Jul 19, 2019

m-orsh commented Jan 9, 2020 •

edited

Loading

		@@ -14,6 +14,19 @@ module.exports.pitch = function (request) {

		var options = loaderUtils.getOptions(this) \|\| {};

		var attrs = {};

feat(index): add data-url attribute #317

feat(index): add data-url attribute #317

Conversation

bukuta commented Apr 20, 2018

jsf-clabot commented Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Apr 20, 2018

codecov bot commented Apr 20, 2018 • edited Loading

Codecov Report

bukuta commented Apr 23, 2018

alexander-akait commented Apr 23, 2018

bukuta commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky May 5, 2018 • edited Loading

Choose a reason for hiding this comment

alexander-akait left a comment • edited Loading

Choose a reason for hiding this comment

michael-ciniawsky commented Aug 28, 2018

adriengcql commented Jul 19, 2019

m-orsh commented Jan 9, 2020 • edited Loading

feat(index): add `data-url` attribute #317

feat(index): add `data-url` attribute #317

jsf-clabot commented Apr 20, 2018 •

edited

Loading

codecov bot commented Apr 20, 2018 •

edited

Loading

alexander-akait Apr 23, 2018 •

edited

Loading

michael-ciniawsky May 5, 2018 •

edited

Loading

alexander-akait left a comment •

edited

Loading

m-orsh commented Jan 9, 2020 •

edited

Loading