Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Add fallback runScript insertion location #144

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

stffrd
Copy link

@stffrd stffrd commented Jun 21, 2016

There's an issue wherein having no script tags on the page (but having the
js in memory executing) causes errors since runScript has no reference
point to inject scripts.

This commit allows for two cases:

  1. If there are no script tags on the page, it will grab the last element
    node in the body and insert scripts next to that element.

  2. If there are no script tags on the page and the body element is EMPTY,
    a script tag will be created and appended to body. Subsequent calls to
    runScript will allow the first use case of inserting next to scripts to be
    called.

This still passes the tests, and I was able to verify the creation of a new script tag after stepping through my debugger and deleting the contents of document.body.

If there's anything I can do to make this better/more acceptable, please let me know! The tests seem sort of nebulous to me at this point? only because I guess there isn't a test suite for runScript ! hahaha.

P.S. My commit that says 'convert to tabs' is actually supposed to be 'convert to spaces'. Oops.

@jdalton
Copy link
Member

jdalton commented Jul 26, 2016

@Morklympious can you rebase this PR?

@stffrd
Copy link
Author

stffrd commented Jul 26, 2016

@jdalton Sure thing, will do later tonight!

@stffrd
Copy link
Author

stffrd commented Jul 26, 2016

And just for my own understanding, @jdalton when you say "rebase this PR", do you mean rebase the changes from upstream's master into my own master and then re-push the change?

Helps me not mess it up in the future!

@jdalton
Copy link
Member

jdalton commented Jul 26, 2016

Yep. To remove the commits unrelated to this PR.

@jdalton jdalton force-pushed the master branch 5 times, most recently from da00022 to e156ebc Compare August 2, 2016 03:11
@jdalton jdalton force-pushed the master branch 2 times, most recently from b08353e to f118abf Compare August 8, 2016 16:47
jdalton and others added 5 commits August 12, 2016 05:59
There's an issue wherein having no script tags on the page (but having the
js in memory executing) causes errors since runScript has no reference
point to inject scripts.

This commit allows for two cases:

1) If there are no script tags on the page, it will grab the last element
node in the body and insert scripts next to that element.

2) If there are no script tags on the page and the body element is EMPTY,
a script tag will be created and appended to body. Subsequent calls to
runScript will allow the first use case of inserting next to scripts to be
called.
@stffrd
Copy link
Author

stffrd commented Sep 14, 2016

Interesting that I rebased and they're showing in my git log locally, but they aren't removed by the PR.

@jdalton jdalton force-pushed the master branch 3 times, most recently from 572f88b to 10319e4 Compare September 26, 2016 04:41
@jdalton jdalton force-pushed the master branch 2 times, most recently from 8b41e34 to ea48e72 Compare October 6, 2016 04:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants