Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove document.write for script path and allow external config #120

Merged
merged 3 commits into from
Dec 4, 2014

Conversation

edg2s
Copy link
Contributor

@edg2s edg2s commented Nov 25, 2014

  1. Use document.getElementsByTag name to find the script's path.
    When used async, this fails by providing the wrong path instead
    of failing catastrophically.
  2. Detect if the script was loaded asynchronously by attaching a load
    event to the body and seeing if it fires.
  3. Evaluate the correct script path when it is requested. This gives
    the user time to manually configuring it using the Papa.SCRIPT_PATH
    global. If this isn't done and the script was loaded async, throw
    a helpful error explaining how to fix.

Fixes #119

1. Use document.getElementsByTag name to find the script's path.
   When used async, this fails by providing the wrong path instead
   of failing catastrophically.
2. Detect if the script was loaded asynchronously by attaching a load
   event to the body and seeing if it fires.
3. Evaluate the correct script path when it is requested. This gives
   the user time to manually configuring it using the Papa.SCRIPT_PATH
   global. If this isn't done and the script was loaded async, throw
   a helpful error explaining how to fix.
@edg2s
Copy link
Contributor Author

edg2s commented Nov 25, 2014

This should remove the need to modify the library. It uses the automatically detected script path if the script was loaded synchronously, otherwise it checks for a global Papa.SCRIPT_PATH, which can be set any time before parse. If it isn't set an exception is thrown.

@mholt
Copy link
Owner

mholt commented Nov 26, 2014

Thanks, I'll be taking a look at this shortly.

@mholt
Copy link
Owner

mholt commented Nov 30, 2014

This doesn't jive too well when Papa Parse is loaded in the <head> of the page; line 159 bombs because document.body doesn't exist yet.

For clarity, what do you think about renaming IS_SYNC to LOADED_SYNC?

@edg2s
Copy link
Contributor Author

edg2s commented Dec 1, 2014

Added a check for document.body (if it's doesn't exist yet it must be synchronous loading) and renamed flag as suggested.

@mholt
Copy link
Owner

mholt commented Dec 2, 2014

Getting there; this is a tricky one. There's another subtle bug. If these conditions are satisfied:

  • The script is loaded asynchronously from the body tag
  • The programmer doesn't specify the SCRIPT_PATH
  • worker: true is set

then the parse will succeed, but without using a worker. This is unexpected (granted, it is a programmer error, but there is no way to know there is an error until too late). In this case, I think an error/exception should be thrown, like on line 1379.

…rown

DOMContentLoaded event fires sooner fixing the body-async case.
If the user reqeusts a worker, try to use one even if no path is set
so that a helpful error is shown.
@mholt
Copy link
Owner

mholt commented Dec 4, 2014

Hey, I can't break this one. Nice work, @edg2s! And thank you.

mholt added a commit that referenced this pull request Dec 4, 2014
Remove document.write for script path and allow external config
@mholt mholt merged commit ec99cc8 into mholt:master Dec 4, 2014
@mholt mholt removed the under review label Dec 4, 2014
@edg2s
Copy link
Contributor Author

edg2s commented Dec 5, 2014

Great, you're welcome. We can now use this lib in Wikipedia's VisualEditor.

@mholt
Copy link
Owner

mholt commented Dec 6, 2014

That's great news. The new Papa Parse website (which I'm still building) will have a "Powered by Papa" section. Would it be alright if I include VisualEditor? The spot can be a link, too, so if there's a particular page you want it to link to, just let me know.

@edg2s
Copy link
Contributor Author

edg2s commented Dec 28, 2014

Of course, I think https://www.mediawiki.org/wiki/VisualEditor is our "homepage" at the moment.

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

Successfully merging this pull request may close these issues.

document.write in getScriptPath throws exception when library loaded asynchronously
2 participants