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

🐛 srcset not working for source-element #987

Closed
mdings opened this issue Mar 12, 2018 · 9 comments · Fixed by #992
Closed

🐛 srcset not working for source-element #987

mdings opened this issue Mar 12, 2018 · 9 comments · Fixed by #992

Comments

@mdings
Copy link

mdings commented Mar 12, 2018

Related to this issue: #720. I'm opening a new one since the other has been closed already.

srcset is still nog working for source-element

Expected behaviour

The asset from the srcset-attribute inside the source-tag is processed by parcel and is rewritten to the hashed path

Current behaviour

The srcset-attribute is left alone.

Code sample

<picture>
    <source srcset="img/gallery/400x600.jpg" media="(min-width: 768px)">
    <img src="img/gallery/800x600.jpg">
</picture>

compiles to:

<picture>
    <source srcset="img/gallery/400x600.jpg" media="(min-width: 768px)">
    <img src="/dist/f44ad9eb3800b52d8c644dd71c6f34d6.jpg">
</picture>
@mdings mdings changed the title 🐛 bug report: srcset not working for source-element 🐛 srcset not working for source-element Mar 12, 2018
@davidnagli
Copy link
Contributor

srcset get’s processed here:

collectSrcSetDependencies(srcset) {
const newSources = [];
for (const source of srcset.split(',')) {
const pair = source.trim().split(' ');
if (pair.length === 0) continue;
pair[0] = this.processSingleDependency(pair[0]);
newSources.push(pair.join(' '));
}
return newSources.join(',');
}

@mdings
Copy link
Author

mdings commented Mar 16, 2018

I noticed this was merged to master so I installed the updated version from github. For me the source-element is still not processed. Am I missing something?

@VladimirVaivada
Copy link

VladimirVaivada commented Mar 16, 2018

🐛 Same issue with parcel-bundler version 1.6.2, node@9.8.0.
Reopen issue?

@gnijuohz
Copy link
Contributor

Hey, sorry! @VladimirVaivada @mdings , I'll look into it soon, meanwhile could you check if this test file is testing the correct thing here?

@gnijuohz
Copy link
Contributor

gnijuohz commented Mar 17, 2018

Okay... just tested it out and it's working for me.

Note that my change was not released in 1.6.2, you need to run a local version of parcel to try it out. Here describes several ways to do that.

One way to do that is,

  1. yarn global remove parcel, to remove globally installed parcel
  2. git clone https://github.com/parcel-bundler/parcel.git
  3. cd parcel && yarn install && yarn build
  4. run yarn link

Now when you run parcel index.html it uses the newly built parcel. @mdings are you certain you were running the correct parcel?

@VladimirVaivada
Copy link

VladimirVaivada commented Mar 17, 2018

😊 My Bad!!! Sorry ))) Did not check if your commit is in current release.
Everything works just fine! 🚀
🥇 Thank you very much!!! 🤗
I'm sorry for my English. )

@mdings
Copy link
Author

mdings commented Mar 19, 2018

@gnijuohz I think so yes. I updated the npm package through the github repo by running npm i -D https://github.com/parcel-bundler/parcel.git.

Now I tried updating the dependencies using yarn, as you are describing above. Still no luck though, but I am guessing at this point this must be something on my side. The integration test-file for one looks good to me.

I'll just wait for the fix to be published to npm and then try updating again.

@gnijuohz
Copy link
Contributor

gnijuohz commented Mar 20, 2018

Hi @mdings I suspect you are somehow still running the version of parcel you installed from before...

Can you check out this example repo?

Run npm install && cd node_modules/parcel-bundler && npm install then cd ../.. && node bundle.js, you should be able to see the result in dist folder (It's interesting that I needed to do cd node_modules/parcel-bundler && npm install though...)

To use cli instead of API, run ./node_modules/parcel-bundler/bin/cli.js index.html

@VladimirVaivada
Copy link

VladimirVaivada commented Mar 20, 2018

@gnijuohz
Hello again! ✋
Fix does work. 👌 with HTML assets.
It does not work with parcel-plugin-pug from @Ty3uK though, but I believe it's a bug with plugin and Maksim will fix it soon. 😄
Thanks for great job! 🍺

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

Successfully merging a pull request may close this issue.

4 participants