Closed GoogleCodeExporter closed 9 years ago
I'm researching the best way to handle "modules" and "packages".
Original comment by ariya.hi...@gmail.com
on 31 Jan 2011 at 3:19
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.
Original comment by dieg...@gmail.com
on 19 Feb 2011 at 7:09
Note that a lot of things are not really possible right now until
http://bugreports.qt.nokia.com/browse/QTWEBKIT-2 is solved.
Original comment by ariya.hi...@gmail.com
on 19 Feb 2011 at 9:54
+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.
Original comment by detroniz...@gmail.com
on 25 Feb 2011 at 2:44
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);
}
Original comment by detroniz...@gmail.com
on 25 Feb 2011 at 2:58
Original comment by ariya.hi...@gmail.com
on 1 Mar 2011 at 6:45
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();
Original comment by ariya.hi...@gmail.com
on 9 Apr 2011 at 4:32
Issue 89 has been merged into this issue.
Original comment by roejame...@gmail.com
on 15 Apr 2011 at 12:35
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 :)
Original comment by detroniz...@gmail.com
on 16 Apr 2011 at 6:13
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.
Original comment by ariya.hi...@gmail.com
on 19 Apr 2011 at 5:53
Until this is resolved - what would be the best way to load a library eg.
jQuery into a loaded page?
Original comment by josscrow...@gmail.com
on 22 Apr 2011 at 3:34
Scroll up and see comment #5 for the workaround.
Original comment by ariya.hi...@gmail.com
on 22 Apr 2011 at 5:26
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);
}
Original comment by ns1...@gmail.com
on 24 Apr 2011 at 4:10
You forgot to add that as a public slot. :)
Original comment by roejame...@gmail.com
on 26 Apr 2011 at 5:57
As shown here at my fork:
https://github.com/detro/phantomjs/blob/major_refactoring/src/phantom.cpp#L394
Original comment by detroniz...@gmail.com
on 26 Apr 2011 at 9:24
@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.
Original comment by detroniz...@gmail.com
on 26 Apr 2011 at 9:35
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');
Original comment by ariya.hi...@gmail.com
on 26 Apr 2011 at 11:14
Hey detronizator, what does updateState do in your snippet from comment #5?
Just console.log a message or something similar?
Original comment by Alex.M.M...@gmail.com
on 11 May 2011 at 4:57
Yes, that snippet comes out of my tests, and was updting the state while
writing some console.log.
Original comment by detroniz...@gmail.com
on 11 May 2011 at 6:57
Original comment by ariya.hi...@gmail.com
on 26 May 2011 at 6:51
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 :)
Original comment by ariya.hi...@gmail.com
on 26 May 2011 at 7:22
[deleted comment]
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/7
f22b94e128b4dee
Original comment by roejame...@gmail.com
on 4 Jun 2011 at 9:55
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?
Original comment by detroniz...@gmail.com
on 7 Jun 2011 at 4:57
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?
Original comment by roejame...@gmail.com
on 7 Jun 2011 at 6:17
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?
Original comment by detroniz...@gmail.com
on 7 Jun 2011 at 6:26
1)
> 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. :)
Original comment by roejame...@gmail.com
on 7 Jun 2011 at 6:47
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
Original comment by detroniz...@gmail.com
on 7 Jun 2011 at 7:05
> 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. :)
Original comment by roejame...@gmail.com
on 7 Jun 2011 at 7:19
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.
Original comment by roejame...@gmail.com
on 7 Jun 2011 at 10:52
Attachments:
Caching the converted coffee script? Do you think is really worth it?
Original comment by detroniz...@gmail.com
on 7 Jun 2011 at 10:56
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.
Original comment by roejame...@gmail.com
on 7 Jun 2011 at 11:24
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
Original comment by detroniz...@gmail.com
on 7 Jun 2011 at 11:28
> 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). :)
Original comment by roejame...@gmail.com
on 7 Jun 2011 at 11:37
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/ce0577adff0f7a28f7b9ef386ec7ae5ea3f5e
63a) 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. ;)
Original comment by detroniz...@gmail.com
on 7 Jun 2011 at 11:45
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) :)
Original comment by roejame...@gmail.com
on 8 Jun 2011 at 3:45
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/3c665681f432601a40e49dc1ae58ea5fe
1399784
Original comment by roejame...@gmail.com
on 9 Jun 2011 at 2:32
Attachments:
Since it is encouraged to have small patches, I would argue just create a pull
request for what's ready, e.g. injectJS, first.
Original comment by ariya.hi...@gmail.com
on 10 Jun 2011 at 12:22
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
Original comment by detroniz...@gmail.com
on 10 Jun 2011 at 12:27
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)
Original comment by ariya.hi...@gmail.com
on 11 Jun 2011 at 10:47
Well, that for me was the next update for the injectJs.
;)
Ivan De Marino
Front End Developer @ Betfair
Sent from my iPhone 4
Original comment by detroniz...@gmail.com
on 11 Jun 2011 at 11:48
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/3c665681f432601a40e49dc1ae58ea5fe
1399784
Original comment by roejame...@gmail.com
on 12 Jun 2011 at 8:02
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).
Original comment by ariya.hi...@gmail.com
on 13 Jun 2011 at 2:44
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. :)
Original comment by roejame...@gmail.com
on 13 Jun 2011 at 5:57
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?
Original comment by roejame...@gmail.com
on 13 Jun 2011 at 9:09
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.
Original comment by detroniz...@gmail.com
on 13 Jun 2011 at 10:51
I will change scriptLookupDir to libraryPath soon.
Original comment by ariya.hi...@gmail.com
on 18 Jun 2011 at 1:42
Renaming is fine.
But I assume for now ( 1.2) it will work as it already does (I.e. One path).
Right?
Original comment by detroniz...@gmail.com
on 18 Jun 2011 at 9:38
One path only for 1.2.
Is there anything left to be done for this issue? Otherwise, we shall close it.
Original comment by ariya.hi...@gmail.com
on 20 Jun 2011 at 3:21
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.
Original comment by detroniz...@gmail.com
on 20 Jun 2011 at 3:33
Original issue reported on code.google.com by
curiousdannii
on 29 Jan 2011 at 8:20