Skip to content
This repository was archived by the owner on May 30, 2023. It is now read-only.

JS programming model #8

Closed
wants to merge 1 commit into from
Closed

JS programming model #8

wants to merge 1 commit into from

Conversation

diegone
Copy link

@diegone diegone commented Feb 18, 2011

Hi Ariya,

first of all thanks for sharing this project. Very very cool and useful.

I hacked around it a little bit to try to make it a little more user friendly and you're welcome to pull my changes if you like.

The main change I wanted to make was to use a callback-style programming model instead of the state machine. Now you can pass a callback as a second parameter of open() and don't have to worry about changing and checking the state.

I also wanted to reverse the exit logic to a more natural "exit unless I tell you so" instead of hanging by default. So now calling exit() is not necessary unless we're in an infinite loop and really want to exit. To change the exit code return can be used. To prevent from exiting, returning a negative value will do the trick.

Other minor changes are:

  • added automatic closure wrapper around script to prevent global leaks
  • added phantom.ctx map to persist context across page loads (like state but map is more flexible)
  • added phantom.addCtxVar() to more easily modify context (instead of whole map replacement)

I tried to keep it as backwards compatible as possible but the logic about exit and closure are slightly incompatible in some cases so I also fixed the broken examples. (although I didn't refactor/optimize them)

I also created a new "local weather" example combining 3 of the other examples to show the use of callbacks and context.

Cheers,
Diego

* added phantom.ctx map to persist context across page loads (like state but map is more flexible)
* added phantom.addCtxVar() to more easily modify context (instead of whole map replacement)
* added optional extra callback parameter to phantom.open() to provide callback function for new page
* changed default behavior to exit by default (to not exit, return a negative exit code)
* added ability to set exit code by using return rather than exit
* added .gitignore file
* fixed examples (because of exit code and automatic closure)
@ariya
Copy link
Owner

ariya commented Feb 19, 2011

Hi Diego,

Thank you so much for your contributions. Here are some (mostly minor) comments.

Generally it is preferable to have separate small commits for each individual change, as opposed to one big commit that contains everything.

For storing object, I consider extending phantom.state to support object persistence (vs only string storage). See http://code.google.com/p/phantomjs/issues/detail?id=34. I believe this would result in cleaner code.

The callback approach for open is definitely something I really like! I consider this is the sanest approach to help solving both http://code.google.com/p/phantomjs/issues/detail?id=32 and http://code.google.com/p/phantomjs/issues/detail?id=41.

I also agree that phantom.exit might not be always necessary. However, it's still useful in a situation where you want to exit the application from a scoped function, e.g. a callback (because 'return' there will just return from the callback function and not the application).

@detro
Copy link
Collaborator

detro commented Mar 1, 2011

Hi.

I agree with Ariya: this patch maybe should be split into 2/3 different smaller patches, to make integration easier and clearer.

A part from that:

  1. I really like the solution of having an "onOpen" event handler: very Javascript-like approach
  2. I having a set of key-value associated with the state object it's a good idea

@diegone: looking forward to see this in.

@ariya
Copy link
Owner

ariya commented May 25, 2011

The open(address, callback) is now implemented in slightly different way. Thanks!

@ariya ariya closed this May 25, 2011
@andronick83 andronick83 mentioned this pull request Mar 15, 2013
This was referenced Jul 1, 2013
@tigerfz tigerfz mentioned this pull request Feb 5, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants