Skip to content
This repository has been archived by the owner on Sep 14, 2021. It is now read-only.

split and rejoin styles in HtmlSplitter and HtmlRejoiner #40

Closed
wants to merge 1 commit into from
Closed

split and rejoin styles in HtmlSplitter and HtmlRejoiner #40

wants to merge 1 commit into from

Conversation

hurst
Copy link
Contributor

@hurst hurst commented Sep 12, 2016

fixes #32

One thing to note is that splitting sources and dependencies without rejoining them will no longer work in a custom build. Previously you could splitSource() and splitDependencies() and still having a working site without calling rejoin() (only javascript files would be external).

Since external stylesheets with an include property aren't supported in any way, calling split() without rejoin() can't be supported by polymer-build.

I'm hoping to see this crisper issue resolved to support external stylesheets in the future: PolymerLabs/crisper#32

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@hurst
Copy link
Contributor Author

hurst commented Sep 12, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@@ -311,8 +311,8 @@ class HtmlSplitter extends Transform {
for (let i = 0; i < scriptTags.length; i++) {
let scriptTag = scriptTags[i];
let source = dom5.getTextContent(scriptTag);
let typeAtribute = dom5.getAttribute(scriptTag, 'type');
let extension = typeAtribute && extensionsForType[typeAtribute] || 'js';
let typeAttribute = dom5.getAttribute(scriptTag, 'type');
Copy link
Contributor

Choose a reason for hiding this comment

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

haha, nice catch

@FredKSchott
Copy link
Contributor

@hurst looks good! Can you add a test? You can use

test('splits and rejoins scripts', (done) => {
as a template.

also /cc @justinfagnani (who originally wrote this logic) for an extra set of eyes

@FredKSchott
Copy link
Contributor

FredKSchott commented Sep 13, 2016

re: splitting without rejoining: as of right now this isn't (and was never) supposed to be supported by splitHtml(). You should continue to use crisper for that.

Is it currently used by https://github.com/PolymerElements/generator-polymer-init-custom-build to do this?

I stand corrected, see @justinfagnani's comment below.

@hurst
Copy link
Contributor Author

hurst commented Sep 14, 2016

The default build from generator-polymer-init-custom-build does not skip the rejoining.

I was using a custom gulpfile in a project that would split without rejoining for CSP compliance. I figured that was not supported but there was some talk of it in this issue for polymer-starter-kit.

Adding a test for this...

@justinfagnani
Copy link
Contributor

regarding splitting w/o rejoining, that was always supposed to be supported so that we could deprecate Cripser.

@@ -356,6 +374,11 @@ class HtmlRejoiner extends Transform {
pred.hasAttr('src')
);

static isExternalStyle = pred.AND(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be used on line 425 but is missing. Will change it to match a <link> tag as well for matching the changes below.

@mhrst
Copy link

mhrst commented Sep 14, 2016

Sorry for the multiple accounts in here. Was in the middle of converting @hurst into an organization for family use and moving everything into a personal account (@nyanmatt)

@FredKSchott
Copy link
Contributor

Bump! Is this blocked by anything? @sorvell I believe there's a question for you above

@justinfagnani
Copy link
Contributor

I talked to Steve, there's no way to make Polymer CSP compliant without style-src: unsafe-inline.

@mhrst
Copy link

mhrst commented Sep 24, 2016

Yes, from what I can tell it looks very difficult to split inline CSS. The gulp build would have to duplicate the functionality of style-util.html and style transformer.html from the polymer project.

If styles can't be externalized, will it still supported to process them in the gulp pipeline for minifying (Issue #32)?

FredKSchott pushed a commit to Polymer/polymer-cli that referenced this pull request Dec 14, 2016
Splitting and rejoining CSS styles is currently blocked in the
polymer-build library (see Polymer/polymer-build#40).
Until that issue is resolved and merged, the CLI can still optimize
inline styles via css-slam.
FredKSchott added a commit to Polymer/polymer-cli that referenced this pull request Dec 15, 2016
* refactor optimize-streams, add better logging

* add temporary InlineCSSOptimizeStream to build process

Splitting and rejoining CSS styles is currently blocked in the
polymer-build library (see Polymer/polymer-build#40).
Until that issue is resolved and merged, the CLI can still optimize
inline styles via css-slam.

* add tests

* update CHANGELOG

* update test
@tony19 tony19 mentioned this pull request Jan 8, 2017
1 task
@FredKSchott
Copy link
Contributor

Superseded by #103

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

Successfully merging this pull request may close these issues.

6 participants