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

use of -D nodejs #4524

Closed
andyli opened this issue Sep 3, 2015 · 14 comments
Closed

use of -D nodejs #4524

andyli opened this issue Sep 3, 2015 · 14 comments
Assignees
Labels
platform-javascript Everything related to JS / JavaScript
Milestone

Comments

@andyli
Copy link
Member

andyli commented Sep 3, 2015

We have a few choices:

default-D nodejs
1support only browsersupport browser and nodejs
2support browser and nodejssupport only nodejs
3support only browsersupport only nodejs
4support browser and nodejssupport browser and nodejs

Given one may want to write a JS lib that works in both browser and nodejs, I would recommend (4). It would be annoying to create separated versions for node/browser.

Currently we have commits that support (2), e.g. 744b018. When people use hxnodejs, it will define -D nodejs and remove browser support... Note that accessing the node API does not imply the code will only run in node. It could be there is a dynamic check, e.g. using js.Browser.supported, and then access the node API only when it is running in node.

(1) makes sense since the node api will only be available when using a haxelib, which will define nodejs. However it means that a generic JS lib written in haxe may not run in node even if it doesn't use any node API... It can happen e.g. for the feature checks in js.html.compat.*, we should use window in browser, but use global in nodejs.

If we really want the ability to specialize for an environment, I would recommend using -D nodejs-only and -D browser-only.

ping @ncannasse @aduros @nadako

@andyli andyli added the platform-javascript Everything related to JS / JavaScript label Sep 3, 2015
@back2dos
Copy link
Member

back2dos commented Sep 4, 2015

I'm not really fond of the -D nodejs_only thing because that then makes you write #if !nodejs_only. It is kind of silly to introduce flags, of which you will always test the negation. Code that will run in all environments should run with nodejs_only also.

Second of all, I think one shouldn't check for js.Browser.supported to determine whether the node API is available. In an nw.js / electron environment, browser APIs are supported, and node APIs are present (and also preferable more often than not).

The APIs are fully orthogonal and it would be cool to just be able to select them. With nodejs that works, because as a haxelib, it is an opt-in by nature. Similarly, it would be great if one could opt-out of browser specific APIs (e.g. with -D dom=no).

There's always the possibility of having a 3rd API become established. The issue with -D xyz_only is that then of the 3 APIs, you always disable two at the same time.

@andyli
Copy link
Member Author

andyli commented Sep 4, 2015

@back2dos Am I correct that you support (4) as shown in the table?

For the -D nodejs-only thing. I meant to use it for specialization that the fenced code will care only about nodejs. e.g.:

#if nodejs_only
// implementation that is specialized for node
#elseif xyz_only
// implementation that is specialized for xyz
#else
// our default implementation
#end

Anyway, I'm not very interested in specialization and it can be done at a later stage if we pick (4).

The js.Browser.supported check example is merely an example. Of course one should use something like if (hasNodeApi) /* access node api */. My point is that the check is done at runtime. That means at compile-time one would still want to use both browser and node api.

@nadako
Copy link
Member

nadako commented Nov 6, 2015

I like (4), but it's unclear to me if we should revert 744b018 or not. If one wants to make a proper node.js module, even if it's meant to work with browser, it should export to exports, not window. Maybe we should make that configurable somehow, but I can't think of how right now (quite busy with work).

@andyli
Copy link
Member Author

andyli commented Nov 6, 2015

Somehow I reverted 744b018 in 681c009.
My point is that window is undefined in node by default, so $hx_exports will become exports instead of window. But feel free to re-introduce 744b018 if you disagree.

@nadako
Copy link
Member

nadako commented Nov 6, 2015

Yeah, but in NW/Electron environment we have both window and exports, right? But we still want to export to exports.

@andyli
Copy link
Member Author

andyli commented Nov 6, 2015

What about just reversing the order of exports and window, i.e. change the value of $hx_exports to

"typeof exports != \"undefined\" ? exports : typeof window != \"undefined\" ? window : typeof self != \"undefined\" ? self : this"

People who want custom behaviors can assign fields to js.Browser.window (or whatever) manually in main().

@nadako
Copy link
Member

nadako commented Nov 6, 2015

Sounds okay to me.

@ncannasse
Copy link
Member

Uhm, I'm not sure: if you run a web app into Electron/NWJS, are you not expecting to export into window as you would do in Chrome ?

@nadako
Copy link
Member

nadako commented Nov 6, 2015

Well, I'm not sure, but I would use the require mechanism (and thus exports) instead of global window fields.

@nadako
Copy link
Member

nadako commented Nov 6, 2015

One could probably require both, but I think exports is a better practice. I'd like someone to confirm/disprove this.

@Simn Simn modified the milestone: 3.3.0-rc1 Feb 23, 2016
@xastor
Copy link

xastor commented Mar 14, 2016

I just got stung by this :-) I'm using webpack require() and found out it returned an empty object for my haxe library. As a side-note the React library exposes to exports in the same situation.

@Simn
Copy link
Member

Simn commented Apr 2, 2016

What's the status here?

@ncannasse
Copy link
Member

Let's go with exports > window if other people think that's a good idea, the NWJS/Electron workflow is indeed quite particular.

@nadako
Copy link
Member

nadako commented Apr 26, 2016

This seems to be a pretty easy fix that can be done for 3.3. Technically, it might be a breaking change, but I doubt that someone relies on that.

@nadako nadako modified the milestones: 3.3.0-rc1, 3.4 Apr 26, 2016
@nadako nadako assigned nadako and unassigned andyli Apr 26, 2016
@nadako nadako closed this as completed in 4152088 Apr 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-javascript Everything related to JS / JavaScript
Projects
None yet
Development

No branches or pull requests

6 participants