upgrade web components of patternfly to custom elements v1 #40

dabeng · 2017-05-12T08:17:51Z

@bleathem @priley86 , would you like to review this PR?

FYI, https://rawgit.com/dabeng/patternfly-webcomponents/ce-v1-dist/index.html

recrsn · 2017-05-12T10:52:07Z

app/app.html

@@ -5,7 +5,8 @@
  <link rel="stylesheet" href="../dist/css/patternfly.css">
  <link rel="stylesheet" href="../dist/css/patternfly-additions.css">
  <link rel="stylesheet" href="../dist/css/patternfly-webcomponents.css">
-  <script src="//cdnjs.cloudflare.com/ajax/libs/webcomponentsjs/0.7.22/webcomponents.js"></script>
+  <script src="https://rawgit.com/webcomponents/webcomponentsjs/master/webcomponents-lite.js"></script>


You may want to use webcomponents-loader.js in place of webcomponents-lite. It will more reflect the real world usage of web components

recrsn · 2017-05-12T10:56:24Z

@dabeng I see you are using the bundled webcomponents-lite.js that comes with pf. It ships the v0 polyfill and thus is inoperable with v1 webcomponents. Use this instead: /~https://github.com/WebComponents/webcomponentsjs#webcomponents-loaderjs

priley86 · 2017-05-12T12:55:14Z

@dabeng great job getting us started w/ v1! It looks like you've updated the WCs to use the new v1 API. Can you please provide your Travis fork / test pages once done? I think we will definitely need to test the v1 components across browsers and see if anything has changed.

bleathem · 2017-05-12T13:00:56Z

Nice work @dabeng. Thanks for the review and suggestion @Agathver!

@priley86, @dlabrecq do we have any suggestions for @dabeng on how to get the tests to run?

dabeng · 2017-05-13T00:19:26Z

@Agathver @priley86 Thanks a lot for your review. Could you give advice about how to get the tests to run? In other words, how to modify karma.conf.js file?

recrsn · 2017-05-13T03:34:39Z

@dabeng the polyfill you are including supports only the web components v0 spec. Use v1 of the webcomponent polyfill via the webcomponents loader I mentioned above. This should make your tests pass if everything else is okay.

You need to download a copy of webcomponents -lite.js v1 from /~https://github.com/WebComponents/webcomponentsjs and reference it inside karma.conf.js

priley86 · 2017-05-14T18:24:46Z

Yep... agreed on the karma updates. I think we can safely move to the @webcomponents/webcomponentsjs package now and remove webcomponentsjs from package.json. That npm package is no longer current. They have now released the former to Bower and NPM.

If you want to quickly test via CDN, it looks like those scripts exist here:
https://cdnjs.com/libraries/webcomponentsjs

dabeng · 2017-05-15T02:03:43Z

@Agathver @priley86 , thank you for your great recommendations, but I still can't get rid of the following errors after I run "gulp test".

Any solutions? I'' appreciate very much 😊

recrsn · 2017-05-15T16:32:59Z

See: ariya/phantomjs#13895
Apparently PhantomJS (a specific version of WebKit JavaScript core) breaks if you define a property called "name" somewhere with { configurable: true }) It's probably fixed on newer WebKit releases, but PhantomJS hasn't been updated in a while.

priley86 · 2017-05-16T15:40:44Z

@dabeng i think we should try to reference the polyfill from node_modules if possible.
priley86@db4e931

I tried testing this branch in Karma w/ Chrome & Phantom JS just now...

A couple observations:

Chrome seems to work fine (as usual) w/ v1 upgrades you've added. Safari seems to work as well (all components). For whatever reason, Tabs seem to break on Firefox now (properly something silly). Everything else in FF seems OK.
Phantom JS does indeed break when loading the v1 polyfill (I'm getting the same errors in debugger in the webcomponents-lite.js file provided). We should probably note this error and open an issue against the webcomponents repo & Phantom upstream repos...

FWIW, we've had so many problems w/ Phantom in the past that I'd be completely in favor of removing this restriction and leaving Phantom. It regularly does not properly represent the other browsers, and that's why it seems a lot of open source projects have moved to tools like Sauce Labs which gives us Selenium...and most importantly, real browsers to test with in an automated way!

It's becoming a barrier to us moving forward w/ the spec...so I'm in favor of removing Phantom. We can of course keep tabs on this issue and revisit it though. We can also try to target xvfb like @Agathver was suggesting, or look into a new tool such as SauceLabs.

thoughts @bleathem ?

priley86 · 2017-05-16T15:55:40Z

further detail on the polyfill breakage...

It's using Set, and es2015 type.
var ma=new Set("annotation-xml ...
/~https://github.com/webcomponents/webcomponentsjs/blob/master/webcomponents-hi-ce.js

We could try polyfilling Set and see where that gets us...
/~https://github.com/medikoo/es6-set

Another thing we may consider is switching off Phantom to Headless Chrome...

https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md
karma-runner/karma-chrome-launcher#111

Thanks to Luke Dary for this suggestion!

bleathem · 2017-05-16T16:38:21Z

Can we look at using headless chrome instead of PhantomJS?
https://developers.google.com/web/updates/2017/04/headless-chrome
https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md

recrsn · 2017-05-17T05:44:13Z

Headless chrome is a great solution. Glad that Chromium is packaging it out-of-the box. But, we have to wait till Chrome 59.

Currently the pf dependencies also bring SlimerJS - which is used for headless testing in Firefox, may be we should be testing in Firefox too.

dabeng · 2017-05-17T12:46:39Z

You are right. In fact, I recommend every developer of patternfly-webcomponents to use the following configuration snippets in karma.conf.js file:

browsers: ['HeadlessChrome', 'FireFox', 'Opera', 'Safari', 'IE'],

At the same time, we need to install the following launchers firstly:

Firefox (launcher requires karma-firefox-launcher plugin)
Opera (launcher requires karma-opera-launcher plugin)
IE (launcher requires karma-ie-launcher plugin)
Safari (launcher requires karma-safari-launcher plugin)

Apparently, installing various browsers can not be imperative, so I didn't do that in this patch.

dabeng · 2017-05-17T13:41:36Z

@priley86 , travis fork isn't generated this time due to the following error:

Authentication failed for 'https://[secure]@github.com/dabeng/patternfly-webcomponents.git/'

recrsn · 2017-05-17T13:46:48Z

Due to some recent security issue regarding accidental leaking of OAuth tokens in Travis build logs, GitHub has revoked all OAuth tokens used in Travis. You need to add a new personal access token in the AUTH_TOKEN environment variable.

The process is highlighted here: /~https://github.com/patternfly-webcomponents/patternfly-webcomponents/blob/master/TESTS.md#enabling-travis-builds

bleathem · 2017-05-17T16:14:21Z

@dlabrecq can you update the OAUTH token here too?

priley86 · 2017-05-17T19:29:05Z

this is great! let's merge...

priley86 · 2017-05-19T02:50:01Z

@dabeng one more minor request before i forget...can we update TESTS.md to reflect new changes for debugging Karma (as you noted in comments above)?

dabeng · 2017-05-21T16:08:06Z

@priley86 , please review TESTS.md :)

priley86 · 2017-05-22T14:16:04Z

thanks for the updates @dabeng ! great job... i think we still need to confirm w/ @dlabrecq if this can be merged and deploy key will work...

dlabrecq · 2017-05-23T17:08:16Z

The AUTH_TOKEN has been updated for patternfly-webcomponents. However, your personal AUTH_TOKEN still needs to be updated in order to run Travis and generate a master-dist branch for personal forks.

dabeng · 2017-05-24T03:00:54Z

@priley86, please review https://rawgit.com/dabeng/patternfly-webcomponents/ce-v1-dist/index.html

priley86 · 2017-05-24T12:50:06Z

looks great @dabeng !

tested these in Safari, Chrome, and FF on my local, all worked fine. Also nice job w/ the pf-tabs fix. I have a downstream addition I'd like to add w/ support for secondary tabs once this is merged.

@dabeng @bleathem @dlabrecq can we merge this now?

bleathem · 2017-05-24T14:42:43Z

Nice work, thanks @dabeng!

recrsn reviewed May 12, 2017

View reviewed changes

Upgrade web components of patternfly to custom elements v1

e717c24

bleathem merged commit 03c8a93 into patternfly-webcomponents:master May 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade web components of patternfly to custom elements v1 #40

upgrade web components of patternfly to custom elements v1 #40

dabeng commented May 12, 2017 •

edited

Loading

recrsn May 12, 2017

recrsn commented May 12, 2017 •

edited

Loading

priley86 commented May 12, 2017

bleathem commented May 12, 2017

dabeng commented May 13, 2017 •

edited

Loading

recrsn commented May 13, 2017

priley86 commented May 14, 2017 •

edited

Loading

dabeng commented May 15, 2017 •

edited

Loading

recrsn commented May 15, 2017 •

edited

Loading

priley86 commented May 16, 2017 •

edited

Loading

priley86 commented May 16, 2017 •

edited

Loading

bleathem commented May 16, 2017

recrsn commented May 17, 2017

dabeng commented May 17, 2017 •

edited

Loading

dabeng commented May 17, 2017

recrsn commented May 17, 2017

bleathem commented May 17, 2017

priley86 commented May 17, 2017

priley86 commented May 19, 2017 •

edited

Loading

dabeng commented May 21, 2017

priley86 commented May 22, 2017 •

edited

Loading

dlabrecq commented May 23, 2017

dabeng commented May 24, 2017

priley86 commented May 24, 2017

bleathem commented May 24, 2017

upgrade web components of patternfly to custom elements v1 #40

upgrade web components of patternfly to custom elements v1 #40

Conversation

dabeng commented May 12, 2017 • edited Loading

recrsn May 12, 2017

Choose a reason for hiding this comment

recrsn commented May 12, 2017 • edited Loading

priley86 commented May 12, 2017

bleathem commented May 12, 2017

dabeng commented May 13, 2017 • edited Loading

recrsn commented May 13, 2017

priley86 commented May 14, 2017 • edited Loading

dabeng commented May 15, 2017 • edited Loading

recrsn commented May 15, 2017 • edited Loading

priley86 commented May 16, 2017 • edited Loading

priley86 commented May 16, 2017 • edited Loading

bleathem commented May 16, 2017

recrsn commented May 17, 2017

dabeng commented May 17, 2017 • edited Loading

dabeng commented May 17, 2017

recrsn commented May 17, 2017

bleathem commented May 17, 2017

priley86 commented May 17, 2017

priley86 commented May 19, 2017 • edited Loading

dabeng commented May 21, 2017

priley86 commented May 22, 2017 • edited Loading

dlabrecq commented May 23, 2017

dabeng commented May 24, 2017

priley86 commented May 24, 2017

bleathem commented May 24, 2017

dabeng commented May 12, 2017 •

edited

Loading

recrsn commented May 12, 2017 •

edited

Loading

dabeng commented May 13, 2017 •

edited

Loading

priley86 commented May 14, 2017 •

edited

Loading

dabeng commented May 15, 2017 •

edited

Loading

recrsn commented May 15, 2017 •

edited

Loading

priley86 commented May 16, 2017 •

edited

Loading

priley86 commented May 16, 2017 •

edited

Loading

dabeng commented May 17, 2017 •

edited

Loading

priley86 commented May 19, 2017 •

edited

Loading

priley86 commented May 22, 2017 •

edited

Loading