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

polymer-bundler breaks css image paths #1988

Closed
jaichandra opened this issue Jul 8, 2017 · 18 comments
Closed

polymer-bundler breaks css image paths #1988

jaichandra opened this issue Jul 8, 2017 · 18 comments

Comments

@jaichandra
Copy link

My application has below project structure:

my-project
* src
  * assets
    * bower_components
      * my-library
        * assets
          * texture.png
        * x-foo
          * x-foo.html
    * elements.html (list of imports pointing to elements in `my-library` folder)

I have a style with below css in my-library folder:

--paper-toolbar: {         
          border-image: url("../assets/texture.png") 1 0 0 0;
}

After I run polymer-bundler on elements.html, I see below url in the bundled file:

--paper-toolbar: {         
          border-image: url("bower_components/my-library/assets/texture.png") 1 0 0 0;
}

When I run this project locally, the final path that browser looks for is
http://localhost:4200/assets/bower_components/my-library/x-foo/bower_components/my-library/assets/texture.png

I think the style urls should not be updated by polymer-bundler.

@usergenic usergenic self-assigned this Jul 10, 2017
@usergenic
Copy link
Contributor

Are you using polymer-bundler on command-line or polymer (aka polymer-cli)? Please provide any version info you can. I'll try and reproduce your symptom.

@jaichandra
Copy link
Author

jaichandra commented Jul 10, 2017

Here's the test repo for replicate above issue: https://github.com/jaichandra/polymer-bundler-test
I am on version 2.2.0 and using command line

@usergenic
Copy link
Contributor

@jaichandra It appears that vulcanize and polymer-bundler have apparently been doing exactly the wrong thing here since... forever, with respect to rewriting the templated <style> tag css contents when inlining imports.

I am really surprised this hasn't been reported as a bug until now. Also, we have four unit tests which are designed to assert it does precisely the current incorrect behavior.

👎

Fixing this immediately.

usergenic referenced this issue in Polymer/polymer-bundler Jul 10, 2017
…m-module> tags of inlined html imports were being rewritten, but were duplicating and thereby foiling the work of the Polymer's own application of the assetpath property of the dom module.
usergenic referenced this issue in Polymer/polymer-bundler Jul 10, 2017
…m-module> tags of inlined html imports were being rewritten, but were duplicating and thereby foiling the work of the Polymer's own application of the assetpath property of the dom module.
@brettpostin
Copy link

I think I've just encountered the same issue using vulcanize 1.15.3.

Project structure:

app
-- elements
----- my-elements.html (bundle root to a bunch of imports including icons.html)
-- images
----- icons
-------- icons.html (a dom-module wrapped stylesheet)

After vulcanized the assetpath of the bundled dom-module is set to ../images/icons/ which is correct. However all of the src() references have also been rewritten from foo.png to ../images/icons/foo.png making the browser look in /images/images/icons/foo.png.

I'm a little confused where vulcanize fits in with the new polymer-bundler? Are they completely separate tools now? Can a fix be applied to it or will i need to migrate to polymer-bundler?

@usergenic
Copy link
Contributor

We recently made a patch to vulcanize as well as polymer-bundler for a different problem and will release that soon. If its not too difficult we'll patch vulcanize for this flaw as well.

However, definitely recommend you move to polymer-bundler. It's got feature parity with vulcanize and a whole lot more. Polymer Bundler is basically the 2.0 or successor to vulcanize. You can use it with the polymer CLI (aka npm install polymer-cli), directly with its own polymer-bundler command-line util or use it programmatically and/or with polymer-build in a gulp file or your own scripts. Check the README for examples.

@usergenic
Copy link
Contributor

@brettpostin vulcanize and polymer-bundler are essentially completely separate. vulcanize is discontinued though a couple of emergency patches are being made. polymer-bundler is a near complete rewrite and is based on polymer-analyzer, supports multi-bundle aka "sharded" build strategies and is much more flexible than vulcanize.

@usergenic
Copy link
Contributor

usergenic commented Jul 11, 2017

FWIW, investigating this issue yielded another related problem with the inlining of stylesheet content inside of dom-modules (inlined stylesheet imports inside dom-modules DO need to have their urls adjusted, but need to incorporate the dom-module assetpath into the resolution of the urls.)

That has also been addressed in PR #580

usergenic referenced this issue in Polymer/polymer-bundler Jul 12, 2017
)

* Fixed issue #579 where url() values inside `<style>` tags inside of `<dom-module>` tags of inlined html imports were being rewritten, but were duplicating and thereby foiling the work of the Polymer's own application of the assetpath property of the dom module.
* Added comment to import utils and removed extraneous argument in its tests
* Added comments to bundler tests about the url rewrite
* Updated to allow rewriteUrlsInTemplates to rewrite style tags in templates.  No style tags in dom-modules are rewritten at all.
* Fixed the rewriting of style content to observe containing dom-module assetpath.
* Fixed a bad test, which was trying to overcorrect urls in inlined styles.  Guarded against infinite loop.
* Fixup the findAncestor function.
* More comments to explain what's going on.
* CHANGELOG updated
@usergenic
Copy link
Contributor

@jaichandra polymer-bundler v2.3.0 was released with this fix. npm install it and try it out :)

@brettpostin
Copy link

@usergenic Thought that was the case, thanks for the clarification.

I have previously attempted to upgrade to polymer-build/polymer-bundler (see here) but due to lack of guidance and time constraints had to postpone.

Are you able to offer any guidance on our specific scenario (i.e. non shell / element project)?

Our current gulp build task simply takes in the root html imports and spits out individual bundle files:

return gulp.src(['bundle1.html', 'bundle2.html', ...])
        .pipe($.vulcanize({
            strip: true,
            inlineCss: true,
            inlineScripts: true
        }))
        .pipe(gulp.dest(...)) 

How would the same be achieved using polymer-bundler?

@brettpostin
Copy link

Failing that a patch to vulcanize would be extremely welcome too!

usergenic referenced this issue in Polymer/polymer-bundler Jul 13, 2017
)

* Fixed issue #579 where url() values inside `<style>` tags inside of `<dom-module>` tags of inlined html imports were being rewritten, but were duplicating and thereby foiling the work of the Polymer's own application of the assetpath property of the dom module.
* Added comment to import utils and removed extraneous argument in its tests
* Added comments to bundler tests about the url rewrite
* Updated to allow rewriteUrlsInTemplates to rewrite style tags in templates.  No style tags in dom-modules are rewritten at all.
* Fixed the rewriting of style content to observe containing dom-module assetpath.
* Fixed a bad test, which was trying to overcorrect urls in inlined styles.  Guarded against infinite loop.
* Fixup the findAncestor function.
* More comments to explain what's going on.
* CHANGELOG updated
@usergenic
Copy link
Contributor

Hey @jaichandra and @brettpostin just a heads up I had to rollback this fix in v2.3.1 because it was a breaking change for polymer 1.x cases I didn't know about.

Bundler 3.0.0 will roll out with this fix shortly, because it is a breaking change.

@usergenic
Copy link
Contributor

but you can still get it if you install polymer-bundler@2.3.0 explicitly.

usergenic referenced this issue in Polymer/polymer-bundler Jul 14, 2017
…th backwards-compatible flag (#588)

* Fix invalid url-rewriting in inlined templated element style tags. (#580)
* Fixed issue #579 where url() values inside `<style>` tags inside of `<dom-module>` tags of inlined html imports were being rewritten, but were duplicating and thereby foiling the work of the Polymer's own application of the assetpath property of the dom module.
* Added comment to import utils and removed extraneous argument in its tests
* Added comments to bundler tests about the url rewrite
* Updated to allow rewriteUrlsInTemplates to rewrite style tags in templates.  No style tags in dom-modules are rewritten at all.
* Fixed the rewriting of style content to observe containing dom-module assetpath.
* Fixed a bad test, which was trying to overcorrect urls in inlined styles.  Guarded against infinite loop.
* CHANGELOG updated
* Make rewriteUrlsInTemplates effect <style> tag rewriting.
@usergenic
Copy link
Contributor

@jaichandra and @brettpostin polymer-bundler 3.0.0 released with the fix back in it.

@madhusudhand
Copy link

I still see the issue with 3.0.1 where as bundling with 2.3.0 works for me.

For the following file structure

bower_components
  ui-components
     images
     my-component
         my-component.html

2.3.0 bundler generates relative reference as follows

<dom-module id="my-component" assetpath="./bower_components/ui-components/my-component/">
  <template>
    <style include="my-component-styles">
    .selector {
       border-image: url("../images/texture.png");
    }
    </style>
  </template>
</dom-module>

where as 3.0.x generates the absolute path resulting a wrong path for the asset as it will be relatively referenced with assetpath of module.

.selector {
  border-image: url("./bower_components/ui-components/images/texture.png");
}

In this case final path turns out to be ./bower_components/ui-components/my-component/bower_components/ui-components/images/texture.png

@jaichandra
Copy link
Author

jaichandra commented Aug 24, 2017

@madhusudhand I tested this example (https://github.com/jaichandra/polymer-bundler-test) and it seems to work fine now with v3.0.1.

@jaichandra
Copy link
Author

@usergenic I came across this path issue again in a different project that has the same structure as in this examplehttps://github.com/jaichandra/polymer-bundler-test. It took a few hours before I could figure out one minor difference between the projects. The other project's bower.json did not had polymer in there dependencies. Rather it had another project (say my-project), which itself had polymer dependency. So bower install would still install polymer and all other dependencies in bower_components. However the path problem reappeared exactly as mentioned in this issue at the top. Adding polymer as an explicit dependency fixed the issue.

Any idea why this is so? I am not able to understand why an explicit polymer dependency is required.

@usergenic
Copy link
Contributor

usergenic commented Nov 9, 2017

FWIW, https://github.com/Polymer/polymer-cli/blob/master/src/build/build.ts#L91 in CLI basically checks which version of Polymer is being used if no explicit rewriteUrlsInTemplates argument is provided and tries to do the right thing. If you have a polymer.json, you can provide in your build(s) that use bundling, an object for the bundle key instead of just bundle: true-- This will allow you to say bundle: {rewriteUrlsInTemplates: true} or bundle: {rewriteUrlsInTemplates: false} depending on what you want.

@jaichandra
Copy link
Author

Thanks.. That helps.

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

No branches or pull requests

5 participants