ChromeDevTools / debugger-protocol-viewer

DevTools Protocol API docsβ€”its domains, methods, and events
https://chromedevtools.github.io/devtools-protocol/
Other
860 stars 173 forks source link

Jekyll to Polymer migration #87

Closed TimvdLippe closed 6 years ago

TimvdLippe commented 6 years ago

This PR moves all Jekyll logic to Polymer custom elements. I could nicely reuse most logic into cr-domain-details, as events, methods and types could fit in this model :tada:

As such, some styling is moved from the global stylesheet into its corresponding shadow dom style. Instead having a file for each category, these are now only listed once in index.html. The rest of the logic and fetching of the protocol data is done here as well. As such, we have less duplication :smile: (this uses ES6 features, so have to look at browser compatibility).

Most of the if statements are transformed into [hidden] attributes. Links and scrolling does not work, as linking to identifiers in shadow boundaries do not work. Once I implement that, I can fix the scrolling issue as well.

The PR management is a bit more tricky, as I don't have a branch upstream to create a PR to. Otherwise, I would need to create a PR to my own fork, which you can't review. I am not sure what the best way forward is. I could merge the build optimizations into this PR and then we can merge as a whole. Would like to hear your thoughts.

I will get a staging environment up soon, but have to eat dinner now :wink:

TimvdLippe commented 6 years ago

Staging environment is up at https://timvdlippe.github.io/devtools-protocol/ The single page application redirect doesn't work yet on GitHub, have to figure that out. That's why direct deep links will not work yet.

TimvdLippe commented 6 years ago

The live version is based on this branch which is the merge of this PR and #86 with various fixes. It also enables full minification, but not bundling as the polymer-bundler removes the base tag. Performance is a lot better, but with bundling I could get the website under 3.5 seconds. Other than that, LightHouse mostly complains about lack of manifest.json and service-worker, but that should be fairly easy to fix later.

All in all, this fixes scrolling support, makes a SPA ( #80 ) and opens up fixing #79

paulirish commented 6 years ago

Staging environment is up at timvdlippe.github.io/devtools-protocol

Very nice! this looks great so far. :D Really nice work Tim

The PR management is a bit more tricky,

aye. i was thinking you'd branch off polymer-build for this one.

Yes right now that means you'd basically PR to your own branch... but i do think we'd still be able to review it. :)

Since there are no shared commits between the two right now i'm not entirely clear on how the two merge.

paulirish commented 6 years ago

also.. i'm prepping for CDS so a little busy for the next few days. maybe @kdzwinel can review, though I believe he's coming here as well.. :)

TimvdLippe commented 6 years ago

@paulirish Yeah I merged them separately (as they were targeting two different changes) into https://github.com/TimvdLippe/debugger-protocol-viewer/tree/polymer-deploy In that branch I fixed some more stuff, so it is kind of a mess 😭 Not sure what easiest is, as all together it basically throws away a whole bunch of code and replaces it with an equivalent in a different manner.

kdzwinel commented 6 years ago

This year I had to skip CDS 😒 But the good news is, I have time to play with this PR!

Some findings: 1) it works great! Awesome work πŸ‘ 1) protocol.json is refetched on every category change (it's loaded from browser cache though, so that's a nitpick really) 1) scroll animation can take a lot of time on narrow screens when a page is v.long - I'd change the strategy so that scrolling never takes more than ~300ms 1) # links are broken screen shot 2017-10-24 at 00 02 21 1) undefined is shown when description is missing screen shot 2017-10-24 at 00 04 11 1) when changing categories scroll position is preserved - IMO it's unexpected 1) clicking on search result behaves as expected, selecting search result with arrows and clicking Enter reloads the app 1) while page is loading it shows 'whoops you hit a 404!' for a second before showing main content

TimvdLippe commented 6 years ago

protocol.json is refetched on every category change (it's loaded from browser cache though, so that's a nitpick really)

Yes I thought about this and initially decided not to cache in memory due to the memory footprint. Should I still go ahead?

scroll animation can take a lot of time on narrow screens when a page is v.long - I'd change the strategy so that scrolling never takes more than ~300ms

It was using the native scrollIntoView with the smooth behavior. I changed it toinstant for now. WDYT?

# links are broken

Oops, yeah copy-paste issue. Fixed :+1:

undefined is shown when description is missing

Fixed :+1:

when changing categories scroll position is preserved - IMO it's unexpected

Fixed :+1:

clicking on search result behaves as expected, selecting search result with arrows and clicking Enter reloads the app

A history.pushState() FTW. Fixed :+1:

while page is loading it shows 'whoops you hit a 404!' for a second before showing main content

I fixed it by not showing the 404, but it flashes a bit now as data is still being loaded. Are you okay with that?

I also fixed the document title, which still contained Jekyll syntax :joy: Commits are https://github.com/ChromeDevTools/debugger-protocol-viewer/commit/a07292ecc700c68153d42435629e96398ae03b81 and https://github.com/ChromeDevTools/debugger-protocol-viewer/commit/0289c864d5b8719518bf7f9d990d82d482a3bb9b

kdzwinel commented 6 years ago

I don't think we need to care about protocol.json refetching if that's a problem to fix quickly πŸ‘Œ

I think that instant scroll works better πŸ‘Œ

Just tested on my phone and got 3 more:

Responding from my email so not sure if attachments will work:

On Oct 24, 2017 12:05, "Tim van der Lippe" notifications@github.com wrote:

protocol.json is refetched on every category change (it's loaded from browser cache though, so that's a nitpick really)

Yes I thought about this and initially decided not to cache in memory due to the memory footprint. Should I still go ahead?

scroll animation can take a lot of time on narrow screens when a page is v.long - I'd change the strategy so that scrolling never takes more than ~300ms

It was using the native scrollIntoView https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView with the smooth behavior. I changed it toinstant for now. WDYT?

links are broken

Oops, yeah copy-paste issue. Fixed πŸ‘

undefined is shown when description is missing

Fixed πŸ‘

when changing categories scroll position is preserved - IMO it's unexpected

Fixed πŸ‘

clicking on search result behaves as expected, selecting search result with arrows and clicking Enter reloads the app

A history.pushState() FTW. Fixed πŸ‘

while page is loading it shows 'whoops you hit a 404!' for a second before showing main content

I fixed it by not showing the 404, but it flashes a bit now as data is still being loaded. Are you okay with that?

I also fixed the document title, which still contained Jekyll syntax πŸ˜‚ Commits are a07292e https://github.com/ChromeDevTools/debugger-protocol-viewer/commit/a07292ecc700c68153d42435629e96398ae03b81 and 0289c86 https://github.com/ChromeDevTools/debugger-protocol-viewer/commit/0289c864d5b8719518bf7f9d990d82d482a3bb9b

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ChromeDevTools/debugger-protocol-viewer/pull/87#issuecomment-338940645, or mute the thread https://github.com/notifications/unsubscribe-auth/AA8JoCi-e8tWpoN1teFHcjvkKgSRKmoEks5svbZVgaJpZM4QBmuS .

TimvdLippe commented 6 years ago

Home page content was broken because I accidentally added 2 class attributes. However, the search performance issues are related to iron-list, which I cant seem to figure out. For some reason, iron-list is rendering way too many items. Have to do a thorough performance review, but have time for that this weekend.

TimvdLippe commented 6 years ago

@keanulee paging you for the above iron-list issue. For some reason, iron-list is physically generating all items, which it obviously should not, since generating 500+ urls completely trashes the performance. You can try it out at https://timvdlippe.github.io/devtools-protocol/ If you type for some example item in the search box, it generates all 533 urls.

We are calling this._setRenderItems(items); to update the array. This was perfectly fine performance-wise in Polymer 1, but in Polymer 2 all performance is lost. I have tried to debug the source of iron-list, but the implementation is barely understandable :cry:

What I did find, however, is that _increasePoolIfNeeded is recursively called a lot of times generating all these items. It seems that this._virtualStart is always 0, thus it never this._clamps the value to a lower value.

The iron-list has a strict height, but is positioned absolute. Maybe that is a clue?

keanulee commented 6 years ago

The DOM structure doesn't seem right - <iron-list> provides its own scroller and needs a fixed height, but here it is put inside a div.scroller which is the scroller and has fixed height (https://github.com/ChromeDevTools/debugger-protocol-viewer/blob/e44bbbad97aabaf0911d4b80fca9086064b1e92e/elements/cr-search-menu/cr-search-menu.html#L92).

TimvdLippe commented 6 years ago

@keanulee Thanks for the pointers! Eventually I decided to remove all scrolling and position absolute magic and just put it in the iron-pages selector.

@kdzwinel The performance is now back to blazingly fast :heart: Shall I open the 3rd PR which is the merge of this and the other PR with some additional fixes?

kdzwinel commented 6 years ago

Searching is fast again! Good work πŸ‘

Found 3 more (sorry!):

TimvdLippe commented 6 years ago

@kdzwinel The bug-pingpong continues! I fixed 2 out of 3 bugs (so almost pong). I have verified the overflowing, but for some reason Chrome on my phone is aggressively caching the website. I am unable to clear the cache, but I verified the fixes on my desktop.

Regarding the header bug: I am not sure. This seems to be Android Chrome behavior. For some reason, it computes the height of the page incorrectly and hides our header beneath the Chrome url bar header. Not sure how to get around this native behavior.

I have also updated the branch to current master and opened #91 as conclusion of all the work.

kdzwinel commented 6 years ago

Keyboard works again πŸ™Œ

Sorry for tormenting you with those 😒 I'd love to jump in and help with debugging, but ATM I'm snowed under with work.

keanulee commented 6 years ago

Would making the header position: fixed fix the last issue?

TimvdLippe commented 6 years ago

Okay, I basically used https://stackoverflow.com/a/33953987/2761676 to get it fixed (positifion: fixed was indeed the solution @keanulee, but I had to apply it to body). It was an Android Chrome URL bar bug. Overflow is now also fixed. Clicking after search works again. Changing domains works again. Bug Pong :smile:

kdzwinel commented 6 years ago

πŸ‘ Last one ( 🀞 ) - it's impossible to search any API other than ToT. e.g. search dialog on 1.2 still searches ToT and redirects you to ToT once something is clicked.

TimvdLippe commented 6 years ago

@kdzwinel Ah speedy reply :tada: Good catch. I did update the protocolSrc accordingly, but the iron-ajax was not triggering. Adding auto luckily fixed that issue! SPA ftw.

kdzwinel commented 6 years ago

@TimvdLippe not sure how this is possible but now https://timvdlippe.github.io/devtools-protocol/ doesn't seem to work in FF and SF. All pages give 404 😒

TimvdLippe commented 6 years ago

@kdzwinel Well, I tested all user paths now (searching, with keys, clicking, etc..) and it should work now :pray: It was a timing issue with FF where custom elements weren't defined just yet. Waiting with whenDefined solved that issue. Really hope we got all issues now, it has been a ride...