Skip to content

Skip shoebox elements when clearing pre-rendered elements#636

Merged
kratiahuja merged 1 commit intoember-fastboot:masterfrom
paulgoetze:fix/shoebox-availability
Jan 17, 2019
Merged

Skip shoebox elements when clearing pre-rendered elements#636
kratiahuja merged 1 commit intoember-fastboot:masterfrom
paulgoetze:fix/shoebox-availability

Conversation

@paulgoetze
Copy link
Copy Markdown
Contributor

Addresses the shoebox issue described in #565 (comment).

@chrism
Copy link
Copy Markdown

chrism commented Nov 24, 2018

Hi @paulgoetze

I've come back to trying to use the shoebox and thanks to this PR I've managed to get it working!

The only thing is, for me, your branch gives me this error...

jQuery.Deferred exception: Array.indexOf is not a function TypeError: Array.indexOf is not a function
...
clear-double-boot.js:31 Uncaught TypeError: Array.indexOf is not a function
at clearHtml (clear-double-boot.js:31)

Which comes from this line

while (nextNode && nextNode !== endMarker && Array.indexOf(shoeboxNodes, nextNode) === -1)

But, by changing it to

while (nextNode && nextNode !== endMarker && !Array.from(shoeboxNodes).includes(nextNode))

It seems to work perfectly.

Should I submit a new PR to supersede this one, or would you prefer to update this one?

Thanks again.

@paulgoetze
Copy link
Copy Markdown
Contributor Author

Hey, @chrism, thanks for testing. I’ll update this PR. No need to create a new one.

@paulgoetze paulgoetze force-pushed the fix/shoebox-availability branch from f202524 to 3fecd83 Compare November 27, 2018 08:43
@paulgoetze
Copy link
Copy Markdown
Contributor Author

The inclusion check is updated to use !Array.from().includes() now.

@kratiahuja
Copy link
Copy Markdown
Contributor

Hi @paulgoetze Thanks for the PR, tests are failing due to a build issue. Could you please help look at it?

@kratiahuja
Copy link
Copy Markdown
Contributor

Tests are failing due to warp-drive-data/warp-drive#5760. THis should be fixed. Let's re-run the job?

@paulgoetze
Copy link
Copy Markdown
Contributor Author

Sure, I’ll rebase against upstream master.

@paulgoetze paulgoetze force-pushed the fix/shoebox-availability branch from 3fecd83 to e883a69 Compare November 29, 2018 22:44
@chrism
Copy link
Copy Markdown

chrism commented Dec 7, 2018

This is great and fixes the shoebox clearing issue, it would be great to get merged!

@paulgoetze
Copy link
Copy Markdown
Contributor Author

@kratiahuja Just curious: Anything that is holding back this PR from being merged? Do you need help with anything related? Seems to be a rather crucial thing since the latest package version is not working with shoebox data.

@kratiahuja
Copy link
Copy Markdown
Contributor

@paulgoetze sorry nothing blocking it. I just didn't get back to this PR. I'll review now and merge and release today.

@kratiahuja kratiahuja merged commit d86bdd3 into ember-fastboot:master Jan 17, 2019
@kratiahuja
Copy link
Copy Markdown
Contributor

v2.0.3 has been published.

@chrism
Copy link
Copy Markdown

chrism commented Jan 17, 2019

Thank you!

@paulgoetze
Copy link
Copy Markdown
Contributor Author

Great, thanks @kratiahuja! 🎉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants