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

feat: add support for empty tags in tag:attribute matching #129

Merged
merged 1 commit into from
Jul 12, 2017

Conversation

nuragic
Copy link
Contributor

@nuragic nuragic commented Jun 30, 2017

Hi!

I'd like to propose a little change that would make possible to match every element that use a specific attribute, without bloating the config file... I work in a project that use <custom-tags> so currently each time we add a new element that want to load an image, we are forced to update the config file... this change would avoid that.

Tests are passing and it's not a breaking change, AFAIK.

Any feedback is welcomed.

Many thanks! ❤️

@jsf-clabot
Copy link

jsf-clabot commented Jun 30, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 30, 2017

Codecov Report

Merging #129 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #129      +/-   ##
=========================================
+ Coverage    96.8%   96.9%   +0.09%     
=========================================
  Files           2       2              
  Lines          94      97       +3     
  Branches       18      18              
=========================================
+ Hits           91      94       +3     
  Misses          3       3
Impacted Files Coverage Δ
index.js 96.55% <100%> (+0.12%) ⬆️

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 432becb...54bd481. Read the comment docs.

@michael-ciniawsky michael-ciniawsky changed the title [Feature] Add support for empty tags in tag-attribute matching feat: add support for empty tags in tag:attribute matching Jul 1, 2017
@michael-ciniawsky michael-ciniawsky self-assigned this Jul 1, 2017
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.

Please add a test aswell 😛

@@ -27,6 +27,8 @@ By default every local `<img src="image.png">` is required (`require('./image.pn

You can specify which tag-attribute combination should be processed by this loader via the query parameter `attrs`. Pass an array or a space-separated list of `<tag>:<attribute>` combinations. (Default: `attrs=img:src`)

If you use `<custom-elements>`, and lots of them make use of a `custom-src` attribute, you don't have to specify each combination `<tag>:<attribute>`: just specify an empty tag like `attrs=:custom-src` and it will match every element.

Copy link
Member

Choose a reason for hiding this comment

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

webpack.config.js

{
   loader: 'html-loader',
   options: { 
      /* Example Config for this here... :) */
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-ciniawsky Test added! You think we should add this config example right here? Or maybe what I added in the README should be better explained? It would be just having attrs=:whatever-attribute as stated above...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the data-src example would be clear enough, also it's already present in the "examples" section.

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jul 3, 2017

Choose a reason for hiding this comment

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

@nuragic Please nevertheless add the comment above as an example under the explanation, just copy & paste it + the correct options please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-ciniawsky Is that ok?

@nuragic
Copy link
Contributor Author

nuragic commented Jul 2, 2017

Sure, will do! Thanks for the review

@nuragic nuragic force-pushed the nuragic-proposal branch 2 times, most recently from 4a3497d to 11dcdd5 Compare July 3, 2017 07:37
README.md Outdated
If you use `<custom-elements>`, and lots of them make use of a `custom-src` attribute, you don't have to specify each combination `<tag>:<attribute>`: just specify an empty tag like `attrs=:custom-src` and it will match every element.

```js
// Example
Copy link
Member

Choose a reason for hiding this comment

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

-// Example
-{
- test: /\.(html)$/,
- use: ['html-loader?attrs[]=:data-src,
-},
-
-// or alternatevely

webpack >= v2.0.0 Options Syntax only please 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heheh cool, removed.

@@ -27,6 +27,27 @@ By default every local `<img src="image.png">` is required (`require('./image.pn

You can specify which tag-attribute combination should be processed by this loader via the query parameter `attrs`. Pass an array or a space-separated list of `<tag>:<attribute>` combinations. (Default: `attrs=img:src`)

If you use `<custom-elements>`, and lots of them make use of a `custom-src` attribute, you don't have to specify each combination `<tag>:<attribute>`: just specify an empty tag like `attrs=:custom-src` and it will match every element.

Copy link
Member

Choose a reason for hiding this comment

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

**webpack.config.js** (L31)

Also add tests + update README to document the new feature.
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.

@nuragic thx

@nuragic
Copy link
Contributor Author

nuragic commented Jul 3, 2017

Thank you! 🙌

@andersevenrud
Copy link

andersevenrud commented Jul 14, 2017

Nice! I was just looking for this, but scratched my head for a good 30 minutes before I realized this was not in the npm release as of now (I was reading README on github) :)

I tested it out and the only problem I found was this matches with indexOf which means it will (in my case) try to parse both of these attributes: data-icon + data-icon-size, whereas I only wanted the former.

I worked around this by changing the second attribute to data-size, but just thought I'd let you know.

@nuragic
Copy link
Contributor Author

nuragic commented Jul 15, 2017

@andersevenrud Hi! Many thanks for trying it out.

it will (in my case) try to parse both of these attributes: data-icon + data-icon-size, whereas I only wanted the former.

I'm not sure if it's a bug or the expected behavior so, please could you paste here 1) your loader config and 2) the part of your HTML involved?

Regarding the master branch not being aligned with the current release... I'm really sorry you loose 30min figuring out why it wasn't in the code... I think it should be fixed ASAP to avoid this kind of issues; luckily the solution is as easy as merging all the pull requests against a dev branch, then merge dev --> master right before the release. 😄 cc @michael-ciniawsky

Thanks!

@andersevenrud
Copy link

@nuragic Here's an example extracted from my code:

webpack.config.js

const path = require('path');
module.exports = {
  module: {
    loaders: [
      {
        test: /\.(png|jpe?g|ico)$/,
        loader: 'file-loader'
      }
    ]
  },
  output: {
    filename: '[name].js',
    path: path.join(__dirname, 'dist')
  },
  entry: [
    path.join(__dirname, 'index.js')
  ]
};

index.js

const data = require('html-loader?attrs[]=img:src&attrs[]=:data-icon!./index.html');
console.log(data);

index.html

<div data-icon="icon.png" data-icon-size="32x32">
</div>

outputs

Hash: 14f0887e575fa9895030
Version: webpack 3.3.0
Time: 202ms
                               Asset     Size  Chunks             Chunk Names
891dece09b415f6ab5b1db7a881d3413.png  7.63 kB          [emitted]
                             main.js  3.16 kB       0  [emitted]  main
   [0] multi ./index.js 28 bytes {0} [built]
   [1] ./index.js 104 bytes {0} [built]
   [2] ./node_modules/html-loader?attrs[]=img:src&attrs[]=:data-icon!./index.html 125 bytes {0} [built]
   [3] ./icon.png 82 bytes {0} [built]
ERROR in ./node_modules/html-loader?attrs[]=img:src&attrs[]=:data-icon!./index.html
Module not found: Error: Can't resolve './32x32' in '/home/anders/Projects/html-loader-test'
 @ ./node_modules/html-loader?attrs[]=img:src&attrs[]=:data-icon!./index.html 1:88-106
 @ ./index.js
 @ multi ./index.js

As you can see it tries to parse both my attributes, because indexOf is a bit "loose" :)

@nuragic
Copy link
Contributor Author

nuragic commented Jul 17, 2017

Ah! I see... :feelsgood:

I'm going to fix it and open a new PR later today.

Thanks for reporting! 👍

@michael-ciniawsky
Copy link
Member

When #133 lands I will try to get someone to publish a new release as soon as possible :)

nuragic added a commit to nuragic/html-loader that referenced this pull request Aug 3, 2017
joshwiens pushed a commit that referenced this pull request Aug 8, 2017
@michael-ciniawsky michael-ciniawsky removed this from the 0.6.0 milestone Jan 8, 2018
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.

4 participants