Skip to content

Support text in node-webkit. #55

Merged
merged 3 commits into from Jul 22, 2013

2 participants

@busykai
busykai commented Jul 1, 2013

Node-webkit allows loading with require in node's context, so just just
skip the node support if it is node-webkit.

@busykai busykai Add support for node-webkit environment.
Node-webkit allows loading with require in node's context, so just just
skip the node support if it is node-webkit.
0799c79
@jrburke
requirejs member
jrburke commented Jul 1, 2013

OK, so to confirm, it reports process.versions.node but also process.versions['node-webkit'], and so even though it reports as node, its fs capabilities cannot be used, and the browser branch should be used, so the node branch should be avoided if process.versions['node-webkit'] is present?

I would be curious to know why the 'fs' pathway would not work in that project.

@busykai
busykai commented Jul 2, 2013

@jrburke Yes. Node-webkit reports both process.versions.node and process.versions['node-webkit']. It also allows to load RequireJS and executes require() in browser context and does not need to enter the node branch.

However, related to your last comment, I actually realize that perhaps a better way is to also check for presence of require.nodeRequire() to make sure that r.js node-style loading is still enabled in node-webkit. Both cases (browser's require and node's r.js) should be treated and it seems like a fair way to distinguish one from another. I'll update the patch.

@busykai busykai Add check for node's style require.
When running in node-webkit, honor both require() and r.js
require.nodeRequire(). Allow entering the node branch of the code in the
latter case.
902cd2b
@jrburke
requirejs member
jrburke commented Jul 2, 2013

I think the previous commit was fine, no need to check for require.nodeRequire as it is implied anyway for using the text loader plugin -- node does not support module loader plugins in this way anyway. So if you want to revert the last change, then I should be fine to merge the pull request.

But I am still curious -- if the point of node-require is to provide the same facilities of node within a webkit browser, I would have expected choosing the node branch for text.js would have been OK. How does it break if the node path in text.js is used?

@busykai
busykai commented Jul 2, 2013

well, it depends on what library is loaded which is controlled by the application. to load both, one would actually have to rename the require function in global context before loading the other. depending on which of the two functions require is pointing at when loading the text.js, it may or may not fail. using node-style require would be a rare case in general, but still might happen.

@busykai
busykai commented Jul 11, 2013

@jrburke what do you think? can this pull request be merged? don't mean to rush the decision, but i use requirejs/text as a submodule and need to decide whether to re-point.

some additional info on the topic: require() name conflict and window and node contexts.

@jrburke jrburke merged commit e7ab4ca into requirejs:master Jul 22, 2013
@jrburke
requirejs member
jrburke commented Jul 22, 2013

Sorry for the delay. Merged, but if difficulties with mixed node and node-webkit loading arise, may be reconsidered. Revved release to 2.0.9.

@busykai busykai added a commit to busykai/brackets that referenced this pull request Nov 24, 2013
@busykai busykai Upgrade requirejs/text to 2.0.10
This version is required to load and run brackets properly on
node-webkit. See requirejs/text#55 for node-specific details.
Summary of changes:
  > rev to 2.0.10
  > Merge pull request #61 from AnSavvides/master
  > Merge pull request #60 from dakota/patch-1
  > rev to 2.0.9
  > Merge pull request #55 from busykai/master
  > Fixes #57 xpcshell: windows FileUtils.File does not like / paths
  > Rev version for 2.0.7
  > Fixes #52, handle non-existent files in node
  > Fixes issue mentioned in comments for requirejs/r.js#221 about Java usage
  > Merge pull request #49 from fsbdev/patch-1
c7764ea
@busykai busykai referenced this pull request in adobe/brackets Nov 24, 2013
Merged

Upgrade requirejs/text to 2.0.10 #6103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.