Function to load scripts / Module support

[curiousdannii]

curiousdannii@gmail.com commented:

It would be good to have a simple function to load additional scripts, maybe like the importScripts() function from Web Workers. You can of course make your own <scripts> now but that's a lot of work which a function could make quite easy.

With this function a sort of "standard library" including things like issue #29 could be included. And of course it would be easier to include regular JS libraries as well.

Disclaimer:
This issue was migrated on 2013-03-15 from the project's former issue tracker on Google Code, Issue #32.
🌟   8 people had starred this issue at the time of migration.

[ariya]

ariya.hi...@gmail.com commented:

I'm researching the best way to handle "modules" and "packages".

 
Metadata Updates

• Status updated: Accepted

[ariya]

dieg...@gmail.com commented:

I think a simple $ = require('jquery', 'http://code.jquery.com/jquery-1.5.min.js') would be nice. The phantom could fetch the url and cache it so that in subsequent callbacks one can simply do $ = require('jquery') and the cached version is return.

Nice to have could be to have a configuration file that maps logical names to urls so they're easier to maintain/upgrade and don't have to be hard-coded (some sort of dependency injection).

Also nice to have would be to have export('myLib', function(){ ...}); so that callbacks can fetch back what they need and we don't need to re-inject the whole script every time.

[ariya]

ariya.hi...@gmail.com commented:

Note that a lot of things are not really possible right now until http://bugreports.qt.nokia.com/browse/QTWEBKIT-2 is solved.

[detro]

detroniz...@gmail.com commented:

+1

This is really needed, as including through "document.body.appendChild()" requires your code to "wait" for the library to be parsed by the Javascript Core first.

[detro]

detroniz...@gmail.com commented:

For devs that are hitting my same barrier and want this feature, here is a temporary fix:

function includeJs(filename, onloadHandler) {
updateState("includeJs: " + filename);

var el = document.createElement('script');

el.type = 'text/javascript';
el.onload = onloadHandler || null;
el.src = filename;
document.body.appendChild(el);

}

[ariya]

ariya.hi...@gmail.com commented:

 

 
Metadata Updates

• Title updated: Function to load scripts / Module support

[ariya]

ariya.hi...@gmail.com commented:

I guess implementing the Require pattern would be the most optimal solution, since developers are already familiar with that, e.g.

var foobar = require('foobar.js');
foobar.doSomething();

 
Metadata Updates

• Label(s) removed:
  • Priority-Medium
• Label(s) added:
  • Priority-High
• Milestone updated: Release1.2 (was: ---)

[ariya]

roejame...@gmail.com commented:

Issue 89 has been merged into this issue.

[detro]

detroniz...@gmail.com commented:

It would be great if someone can express a preference of which library we should use to implement this pattern.
I found this interesting: http://requirejs.org/docs/jquery.html.
But there are TONS of Javascript Dependency Management Scripts out there.
Some are really big, some are really smart.

What do you think should we pick?

OR... we could just design something simple but effective ourself and implement that.
Do you have a page/reference on how exactly the "require()" pattern you mention works?
I could find multiple example online, and they sometime variate too much.

Yes, I'm volunteering in doing this :)

[ariya]

ariya.hi...@gmail.com commented:

I prefer something simple, i.e. it could be just the logical extension of your script loader.

Starting from an example, something like this would be very reasonable:

var crypto = require('crypto');
var fingerprint = crypto.md5sum(some_data);

Also, the trick is the "exports" pattern inside the module itself, i.e. a module will not simply pollute the global object.

[wjcrowcroft]

josscrow...@gmail.com commented:

Until this is resolved - what would be the best way to load a library eg. jQuery into a loaded page?

[ariya]

ariya.hi...@gmail.com commented:

Scroll up and see comment #5 for the workaround.

[ns1000]

ns1...@gmail.com commented:

In addition to 5 you can also do this:

void Phantom::loadLibrary(const QString &fileName)
{
QFile file;
file.setFileName(fileName);
if (!file.open(QFile::ReadOnly)) {
std::cerr << "Can't open " << qPrintable(fileName) << std::endl << std::
endl;
exit(1);
return;
}
m_library = file.readAll();
file.close();
m_page.mainFrame()->evaluateJavaScript(m_library);
}

[ariya]

roejame...@gmail.com commented:

You forgot to add that as a public slot. :)

[detro]

detroniz...@gmail.com commented:

As shown here at my fork: https://github.com/detro/phantomjs/blob/major_refactoring/src/phantom.cpp#L394

[detro]

detroniz...@gmail.com commented:

@ariya: so, the 'required' pattern is defined in CommonJS as you probably know (http://www.commonjs.org/). And, as you also said, it requires specific 'exports' in the module itself. All good: clearly CommonJS is the future in terms of "standard library" for Javascript.

But, that is a very specific requirement: jQuery or other libraries are not going to change because of CommonJS (for now I guess). Hence, we still lack a built in way to load JS libraries.

[ariya]

ariya.hi...@gmail.com commented:

If we follow the supervisor-worker pattern, what you want there is to "inject" any JS library, correct?

Maybe the example would look like:

var page = WebPage.open(url);
page.injectScript('/path/to/extra/lib.js');

[alexmcpherson]

Alex.M.M...@gmail.com commented:

Hey detronizator, what does updateState do in your snippet from comment #5? Just console.log a message or something similar?

[detro]

detroniz...@gmail.com commented:

Yes, that snippet comes out of my tests, and was updting the state while writing some console.log.

[ariya]

ariya.hi...@gmail.com commented:

 

[ariya]

ariya.hi...@gmail.com commented:

Ivan, with the new WebPage object now you have the chance to implement the above injectScript solution, perhaps extending with an optional callback (since it is asynchronous).

Patch is welcomed :)

[ariya]

roejame...@gmail.com commented:

We were discussing a way to do sequential actions (instead of the verbose async), and found that there are some nice libraries that allow to do this without blocking async. (steps, promise.js, async.js, futures, ..). This would also allow you to split your JavaScript's up into separate modules (could be really useful). So for these reasons I am suggesting we also allow injecting a local JavaScipt into the Phantom object.

And of course, would also love to soon see a way to inject a local / URL based script into a page.

I'd implement it myself were it not for my lack of C++ skills, although I could probably hack something together, I'd rather let someone who knows more do a better job! :)

http://groups.google.com/group/phantomjs/browse_thread/thread/eebcd758f18ad8c4/7f22b94e128b4dee

[detro]

detroniz...@gmail.com commented:

Just submitted a pull request for "page.loadJsFile()" that works with local paths.

Based on the last previous comment and Ariya answer to my pull request, I'd:

• rename to "inject"
• build a support for "URL" (even though this will either be "blocking" or "async")

What do you guys think?

[ariya]

roejame...@gmail.com commented:

I like the name "injectJS", and for the URL JS loader "includeJS".

Did you read my comment #23? There are a few good reasons to also expose "injectJS" to the Phantom object (easy to do, just do a m_page.injectJS(filename)).

Lastly, regarding URL paths, and I see this as a problem; if you use local paths without appending the original script's directory to the path, it becomes like:

./phantomjs ~/Desktop/script.js

script.js:
var page = new WebPage();
page.injectJS('library.js');

The problem with this, is that when it searches for library.js, it'll look inside the phantomjs binary folder (or whatever the cwd is). So, if 'library.js' is in the same folder as script.js, you'll have to do:

page.injectJS('/home/user/Desktop/library.js');

This is why I am proposing to prepend the original scripts directory path to the filename before injecting it. The prepend can be omitted if they already have a directory path in the filename (but first make sure the directory path in the filename ISN'T a subdirectory of the folder script.js is in (e.g. a filename like 'libs/script.js'), otherwise we SHOULD prepend it still)

Does that make sense?

[detro]

detroniz...@gmail.com commented:

1. so, if I get it right, you'd like "injectJS" to be synchronous and only local - "includeJS" to be async (using the <script> tag?)

2. About the paths, I think you are right. The injectJs should prepend the "starting script path" if what's passed is not already a full path. This makes perfect sense for me.
   I did actually think about it while coding today, but because I was not sure of the best approach, I though I'll show a first implementation, and than tune it based on feedbacks

Ariya?

[ariya]

roejame...@gmail.com commented:

you'd like "injectJS" to be synchronous and only local
I thought it was already synchronous and local?

"includeJS" to be async (using the <script> tag?)
I was thinking along the terms of your original includeJS, like..
https://github.com/ariya/phantomjs/pull/11/files#L0R54

-- I really had already assumed this would function similarly to your pull request 11 (link above).

2. Awesome. :)

OH, I almost forgot! When I had the loadScript function on my port, I made it so that people could also load CoffeeScript's as well as JS. It was a nice touch, considering we can already use CoffeeScript's for the main script. Not terribly needed, but would be cool. :)

[detro]

detroniz...@gmail.com commented:

I was just re-describing the 2 methods. "loadJsFile" (what is going to be
"injectJs") is already synchronous.

About the coffee thing, I will be able to do it only for "injectJs" I think.

Ivan De Marino
Front-End Developer @ Betfair

email: ivan.de.marino@gmail.com | detronizator@gmail.com |
ivan.demarino@betfair.com
web: blog.ivandemarino.me | www.linkedin.com/in/ivandemarino |
twitter.com/detronizator
mobile: +44 (0)7515 955 861

[ariya]

roejame...@gmail.com commented:

I was just re-describing the 2 methods. "loadJsFile" (what is going to be
"injectJs") is already synchronous.
Ah, I see.

About the coffee thing, I will be able to do it only for "injectJs" I think.
Right, there's little/no use for doing it on includeJS anyways. As far as the callbacks go, if you specify a callback in CoffeeScript (in the main file), it'll already have been converted by the time it hits includeJS, so that's a plus. :)

Alright, sounds good. Can't wait to port it over. :)

[ariya]

roejame...@gmail.com commented:

I remembered something. I think we should also cache the script. This wouldn't do much for regular js files (we wouldn't have to re-open it again though), but for CoffeeScript's it would be dramatic, since it wouldn't need to be reconverted every single time.

Caching with a key:value based system works, where the key is the original filename passed to the script (not the one we altered with adding the start script). I already have my implementation done. I'll post the diff below.

[detro]

detroniz...@gmail.com commented:

Caching the converted coffee script? Do you think is really worth it?

[ariya]

roejame...@gmail.com commented:

I believe so. My reasoning is:

On each page request, the javascript would have to be re-executed in the page in order to work. This is fine for regular javascript, but with CoffeeScript, it would really slow down page loading. Since we already converted it once, we shouldn't need to redo it every time, since we already did it.

[detro]

detroniz...@gmail.com commented:

Rest assured I see why you want to do it.
It's just that it's an "how many times would we need it" doubt.

Would anyone write a script that loads the same coffee script a lot of
times?

Well, if so, it's a very badly done implementation if you ask me...

Ivan De Marino
Front-End Developer @ Betfair

email: ivan.de.marino@gmail.com | detronizator@gmail.com |
ivan.demarino@betfair.com
web: blog.ivandemarino.me | www.linkedin.com/in/ivandemarino |
twitter.com/detronizator
mobile: +44 (0)7515 955 861

[ariya]

roejame...@gmail.com commented:

It's just that it's an "how many times would we need it" doubt.
Would anyone write a script that loads the same coffee script a lot of
times?
True, I just didn't see any negligible effects, since it seems fairly easy to implement and the function exits much more quickly (instead of having to re-open the file all the time).

The Phantom object has the effect of caching the script (in it's own way) in m_script.

I guess it's a, "do the benefits outweigh the costs" scenario.

Do you think there are any negligible effects to implementing something like that?

Sorry, not trying to overload you here, just trying to make sure the implementation covers all bases (like in the case of the local file path loading). :)

[detro]

detroniz...@gmail.com commented:

I'd go to the extent of saying that the caching should be in the "CSConverter" class.
I'll see where does that fits.

I'm working on the "includeJs" as well now, and it is slightly trickier than before, given that the whole thing is now split in 2 different contexts, while before there was only one.
I'm trying a couple of approaches before submitting my solution.

In the meantime though, take a look to this commit (https://github.com/detro/phantomjs/commit/ce0577adff0f7a28f7b9ef386ec7ae5ea3f5e63a) to see if you think our implementation is equivalent for "injectJs".

Of course, bear in mind that the Coffee Script support is not there yet. ;)

[ariya]

roejame...@gmail.com commented:

scriptLookupDir() looks nice. Changed mine accordingly.

They seem to be functionally equivalent, minus all my added stuff of course. (CoffeeScript, caching, injectJs in Phantom object) :)

[ariya]

roejame...@gmail.com commented:

After doing some speed tests, I've determined that caching is (almost) negligible. I found the cause of the speed slowdown, and why caching it was speeding it up.

The reason was that the Converter was the one that needed to be cached (which is really easy), rather than the script. :)

Here's my new diff.

P.S. Fixed a bug with the CoffeeScript converter.
https://github.com/Roejames12/phantomjs/commit/3c665681f432601a40e49dc1ae58ea5fe1399784

[ariya]

ariya.hi...@gmail.com commented:

Since it is encouraged to have small patches, I would argue just create a pull request for what's ready, e.g. injectJS, first.

[detro]

detroniz...@gmail.com commented:

I will tomorrow. Today I implemented inject and include, but tonight I
couldn't submit the pull.

If curious, check out my "utilities" branch: there is a commit with
questions that you might be able to answer ;)

Ivan De Marino
Front End Developer @ Betfair

Sent from my iPhone 4

[ariya]

ariya.hi...@gmail.com commented:

Here is an idea is someone wants to pursue it (and beats me to it): a native implementation of includeJS. We can use the network access manager to fetch the script.

Benefit: no need to "pollute" the page DOM (with the script tag)

[detro]

detroniz...@gmail.com commented:

Well, that for me was the next update for the injectJs.
;)

Ivan De Marino
Front End Developer @ Betfair

Sent from my iPhone 4

[ariya]

roejame...@gmail.com commented:

Looks great! However one thing. The bug I had fixed in this commit is now present again. This only needs to be done to js files, because in CoffeeScript # is a comment, so it'll be ignored. However prepending // for CoffeeScript does not cause it to be a comment, but rather a RegExp (which is unintended!). I'll fix it on my next pull request.

https://github.com/Roejames12/phantomjs/commit/3c665681f432601a40e49dc1ae58ea5fe1399784

[ariya]

ariya.hi...@gmail.com commented:

What do you guys think of the name "libraryPath."

Rationale: seems that we will just include scripts here, I can't foresee PhantomJS including/injecting anything else. Therefore the "script" prefix is not necessary.

Personally, I also prefer "path" since it seems to be more general.

In addition, if we would support multiple paths (for different lookups), maybe it should be "libraryPaths" with a possible value of array of string (instead of only a single string).

[ariya]

roejame...@gmail.com commented:

Supporting multiple paths could be a very good idea (operating similar to the PATH environment variable, able to have multiple paths). Definitely useful for having multiple folders of libs elsewhere on the system.

I'm fine with a rename, libraryPaths seems good. :)

[ariya]

roejame...@gmail.com commented:

What does everyone think about a global "libraryPath", instead of 1 per page object/and the phantom object?

Perhaps we could keep 1 per page object/and the phantom object, and still have a global one?

Is it a good idea?

[detro]

detroniz...@gmail.com commented:

I'd say, for sake of simplicity, "libraryPaths" for page objects are ALREADY generated out of the same one (i.e. the phantom object's libraryPaths).

Having a global one isn't really going to make a lot of difference.
Plus, I confess I like the isolation.

I have in the pipe (once I get some proper home-time to do a nice after-work coding session) a small little library based on Qt. It's going to simplify how to handle command line arguments/options. Something like "getopts", but in a QSettings style.
My aim is to than integrate this thing in Phantom, and so make very easy to pass some "configuration parameters" to the phantom object directly from the CLI.

[ariya]

ariya.hi...@gmail.com commented:

I will change scriptLookupDir to libraryPath soon.

[detro]

detroniz...@gmail.com commented:

Renaming is fine.
But I assume for now ( 1.2) it will work as it already does (I.e. One path).
Right?

[ariya]

ariya.hi...@gmail.com commented:

One path only for 1.2.

Is there anything left to be done for this issue? Otherwise, we shall close it.

[detro]

detroniz...@gmail.com commented:

I have 2 tasks on my TODO list:

• Implement an asynchronous "injectJs"
• Add support for multiple entries in the "PATH"

But that can be part of 1.3, as the current stuff in 1.2 is good enough for me.

[ariya]

ariya.hi...@gmail.com commented:

What about native version of includeJS?

[detro]

detroniz...@gmail.com commented:

Well, the "callback" part can be done only on the JS side.
The appending of the DOM Element "could" be done in C++ for sure but... do we really have to?
The amount of JS that is executed there is minimal, and I can't see a real benefit in doing it native, manipulating QWebElements. Maybe I'd save few bits of memory and it would be faster because there would be no JS->C++ "translation" but... really? :)

Unless there is something I completely miss.

[ariya]

ariya.hi...@gmail.com commented:

If it is possible to implement something without touching the page document, that's the preferred way to do that. I already mentioned the use of network access manager to fetch the script. I'm sure the callback problem can be solved as well.

[detro]

detroniz...@gmail.com commented:

I tried using the Network Access Manager in my first attempt actually, and the problems was that there is a (for obvious reasons) a delay between "last bit of the resource received" and "resource loaded".

As fast as the JS engine can be, it's still slow enough to break this approach.

That's why I ended up "temporary using" the page signal->slot. And, after all, it's not like anything else is affected, isn't it?

[ariya]

ariya.hi...@gmail.com commented:

Not sure what you mean by the delay there. Care to elaborate?

Also, your approach is touching the document/DOM. If there is another way to implement the same thing without changing the page itself, that'll be much preferable.

Meanwhile, I'll close this one and we continue on native implementation as a separate issue.

 
Metadata Updates

• Status updated: Fixed

[detro]

detroniz...@gmail.com commented:

Are we talking about "includeJs" right?

IncludeJS purposely adds a <script> tag to the DOM. That was the purpose.
To include stuff without touching the dom, there is the synchronous "injectJs".

Or are we talking about something else here?

Anyway, where do we pick this conversation up?

[ariya]

ariya.hi...@gmail.com commented:

If the intention of includeJS really to modify the DOM by adding the script tag, then it should be fine.

I guess one day we need to split all these convenience functions to its module, instead of stashing them in bootstrap.js.

[detro]

detroniz...@gmail.com commented:

yes, but the problem is that it needed a mix of JS and Native code.
If it was pure JS...

Ivan De Marino
Front-End Developer @ Betfair

email: ivan.de.marino@gmail.com | detronizator@gmail.com |
ivan.demarino@betfair.com
web: blog.ivandemarino.me | www.linkedin.com/in/ivandemarino |
twitter.com/detronizator
mobile: +44 (0)7515 955 861