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

Harmony in node-wekbit? #298

Closed
mblinsitu opened this issue Dec 29, 2012 · 29 comments
Closed

Harmony in node-wekbit? #298

mblinsitu opened this issue Dec 29, 2012 · 29 comments
Milestone

Comments

@mblinsitu
Copy link

I would like to use the Harmony features of V8 in node-webkit.
I can do that with node alone using "node --harmony" but this option is not recognized by node-webkit.

I am particularly interested in Proxy and WeakMap. As explained in issue #296, the node-proxy module that emulates Proxy does not work out of the box with node-webkit on the Mac because of 32- vs. 64-bit issues.

More generally, it would be useful to expose the V8 engine options to node-webkit since these are available from both node and webkit (e.g. in Chrome using the about:flags URL).

@Mithgol
Copy link
Contributor

Mithgol commented Dec 29, 2012

It also could make some things easier if these options were supported as node-webkit's package.json fields in addition to (maybe even instead of) the command line's switches. (After all, package.json is where we define the application's properties and required features.)

@mblinsitu
Copy link
Author

I agree. It would be much better to configure these through package.json rather than with command-line arguments.
Maybe with a "V8-options" field? Ideally, all the options listed with node --v8-options should be available.

@Mithgol
Copy link
Contributor

Mithgol commented Jan 3, 2013

The above idea is specified in the #307 issue now, so this issue (#298) may be used exclusively for tracking the future Harmony command line features.

@rogerwang
Copy link
Member

Chromium has a command line flag js-flags to set this. Since we added the support for Chromium args in manifest with #307 , this issue is also fixed now. You can use harmony-proxies in js-flags.

@zhchbin
Copy link
Contributor

zhchbin commented Jan 15, 2013

Hi @rogerwang . It seems that currently my implement of the chromium-args can't work in this situation. I test it on WIN . Sorry for the miss. Do you have any idea about this?

@rogerwang rogerwang reopened this Jan 16, 2013
@rogerwang
Copy link
Member

it doesn't handle key=value case. I'll have a fix soon.

@rogerwang
Copy link
Member

@zhchbin can you review my changes to see whether it's ok for you?

@Mithgol
Copy link
Contributor

Mithgol commented Jan 16, 2013

Would you extend the corresponding wiki article with an example of js-flags in chromium-args, maybe with an example of Harmony flags specifically? That could save the users from reading a manual on chromium command line arguments for this (relatively common) case.

@rogerwang
Copy link
Member

@Mithgol done and thanks for the suggestion. https://github.com/rogerwang/node-webkit/wiki/Manifest-format/_compare/fc6d2c8%5E...fc6d2c8

I'm not sure why the syntax highlighting is not working...

@zhchbin
Copy link
Contributor

zhchbin commented Jan 16, 2013

Hi, @rogerwang, thanks for fix my implement, key=value case is missed out of ignorance. However, On WIN, there will be a compile error because CommandLine::StringType will be std::wstring after your change.

Edit: and it break the use case like: "chromium-args" : "disable-accelerated-video". Now it should be "chromium-args" : "--disable-accelerated-video". However, I think your choice would be better.

zhchbin added a commit to zhchbin/node-webkit that referenced this issue Jan 16, 2013
rogerwang added a commit that referenced this issue Jan 16, 2013
[WIN] Fix compile error because CommandLine::StringType will be std::wstring. #298
@Mithgol
Copy link
Contributor

Mithgol commented Jan 16, 2013

The "chromium-args": "--js-flags=--harmony-proxies" example is good and may suffice.

Though I still wonder if a newbie would need a more complex example involving escaped quotes and a separator between two different V8-related arguments, such as --harmony_proxies and --harmony_collections:

"chromium-args": "--js-flags=\"--harmony-proxies --harmony_collections\""

I haven't made any decision about its necessity (otherwise I would have replaced the example in the wiki already).

@zhchbin
Copy link
Contributor

zhchbin commented Jan 16, 2013

@Mithgol Sorry to tell you that, currently my implement doesn't meet your case:

"chromium-args": "--js-flags=\"--harmony-proxies --harmony_collections\""

The correct one should be:

"chromium-args": "--js-flags=--harmony-proxies --js-flags=--harmony_collections"

I will update the wiki...

Update: In the previous use case, only the last one js-flags work.

@pauliusuza
Copy link

I'm trying to use this on v4.0:
"chromium-args": "--js-flags=--harmony-proxies --js-flags=--harmony_collections --js-flags=--harmony_modules --js-flags=--harmony_scoping"

but I keep getting errors in my app.html:
Uncaught ReferenceError: WeakMap is not defined

any ideas?

@rogerwang
Copy link
Member

try to add harmony-weakmaps to the js-flags

@pauliusuza
Copy link

Tried it, no luck. Does it work for anyone else?

[edit: using single flag --js-flags=--harmony solves the issue. thanks @zhchbin!]

@zhchbin
Copy link
Contributor

zhchbin commented Jan 25, 2013

@pauliusuza Please have a try by using: "chromium-args": "--js-flags=--harmony" This will enable all.

@rogerwang It seems that using space to separate two option in our use case does have some problems. Can we change it to use another? For example: ; And my previous comment is wrong because I just test whether the second option work. In the following use case, only the last work..
"chromium-args": "--js-flags=--harmony-proxies --js-flags=--harmony_collections --js-flags=--harmony_modules --js-flags=--harmony_scoping"

@owenc4a4
Copy link
Contributor

Can this be the solution? fc297de352156fedaa7bacd145e8b078a006c535
using '|' to separate the value.

@zhchbin
Copy link
Contributor

zhchbin commented Jan 29, 2013

Hi @owenc4a4 , maybe wrong, your commit has nothing to do with the case in my comment... I will fix it as my previous description and update the test case and wiki soon.

@rogerwang
Copy link
Member

Why we cannot use whitespace as the separator?

@zhchbin
Copy link
Contributor

zhchbin commented Jan 30, 2013

@rogerwang in the following case:
"chromium-args": "--js-flags=--harmony-proxies --js-flags=--harmony_collections",
only --js-flags=--harmony_collections work, the first one will be overrode. Change to other separator, like ; can fix this, but it will change the api, this is why am I waiting for your opinion.

The new usage of this api will become
"chromium-args": "--js-flags=--harmony-proxies --harmony_collections;--disable-accelerated-video"

@rogerwang
Copy link
Member

I prefer not to use semicolons. Can multiple js-flags be quoted with single quote (') ?

@zhchbin
Copy link
Contributor

zhchbin commented Jan 30, 2013

I get what you are concerned about. However, if multiple js-flags be quoted with single quote ', there still have some spaces between every two options within single quote, to deal with this I should change this simple code to more complicated one.

@rogerwang
Copy link
Member

I think there could be some code for this kind of command line parsing in Chromium code which we could utilize.

@Mithgol
Copy link
Contributor

Mithgol commented Jan 30, 2013

Alternatively, you may make these flags separate by introducing some "js-flags": "..." property in the manifest (with space-separated flags inside the property's value) that would be combined with chromium-args internally (inside the node-webkit engine) and then applied.

That would be easier for the people composing their manifests (I mean, not to mistype space for a semicolon out of habit, or a double quote instead of single, etc.).

Some visual separation between the flags of different nature (where chromium-args affect only Chromium while js-flags affect all JavaScript, including Node.js) feels somewhat natural.

@zhchbin
Copy link
Contributor

zhchbin commented Jan 30, 2013

@rogerwang will change the code as your description. Reopen to track it.

@zhchbin zhchbin reopened this Jan 30, 2013
@ghost ghost assigned zhchbin Jan 30, 2013
@rogerwang
Copy link
Member

I think @Mithgol 's idea is great. You can add that field in manifest.

@zhchbin
Copy link
Contributor

zhchbin commented Jan 30, 2013

Got it, thanks @Mithgol 's idea.

@Mithgol
Copy link
Contributor

Mithgol commented Jan 30, 2013

Thanks for accepting my suggestion.

@zhchbin
Copy link
Contributor

zhchbin commented Jan 31, 2013

Wiki of js-flags and chromium-args updated: Link

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

No branches or pull requests

6 participants