Skip to content
This repository has been archived by the owner on Feb 26, 2022. It is now read-only.

cfx-js integration work #578

Closed
wants to merge 9 commits into from
Closed

cfx-js integration work #578

wants to merge 9 commits into from

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Sep 17, 2012

This is work in progress pool of changes required for integrating cfx-js linker

@ghost ghost assigned ochameau Sep 17, 2012
@Gozala
Copy link
Contributor Author

Gozala commented Sep 17, 2012

  1. I have diced that probably it would be best to just nodes path module instead of reimplementing one. It's well tested on node and considered stable. Also this way behavior will be exactly the same as in node.
  2. Unlike node cfx-py does not recognizes relative paths from test modules to an enclosing package modules, which sucks, 1. as one is unable to write tests that work on both platforms, 2. I had to rewrite all my tests to update require paths, which would also break them for node.
    So I have decided to change cfx to add support for this (hopefully it's not very hacky).

@Gozala
Copy link
Contributor Author

Gozala commented Sep 18, 2012

  1. Ported jetpack-io fs module to the core to have node like fs API. I have not took to much time in polishing it since it's a temporary implementation until we get new async OS.File API on main thread. On the way I have made some changes to the heritage module.

@ochameau
Copy link
Contributor

  • +1 on using node's path module!
  • I'm fine with the cfx-py hack as soon as it will disappear in JS/packageless version, right?
  • About OS.File, the last patch should land soon. David came back from his "newly dad" days off ;) And the last patch is r+ and it is only matter of landing it. He told me he should do through bug 777711 this week.
  • If we ship fs temporary, It would be better to put it in cfx folder.
  • I'm more concerned about streamer dependency than this temporary nodejs fs implementation, but that may not be relevant in this pull request?

@Gozala
Copy link
Contributor Author

Gozala commented Sep 18, 2012

I'm fine with the cfx-py hack as soon as it will disappear in JS/packageless version, right?

Yes of course !

About OS.File, the last patch should land soon. David came back from his "newly dad" days off ;) And the last patch > is r+ and it is only matter of landing it. He told me he should do through bug 777711 this week.

That's awesome! Although I would prefer to get cfx-js out of the way and then do the rewrite of fs module.

If we ship fs temporary, It would be better to put it in cfx folder.

In fact I think we should ship fs module in the toolkit. I chose api-utils/fs because then I'm able to use require('fs')
putting it in cfx would mean require('./fs') which will no longer be compatible with node, so I won't be able to use
it in node etc...

I'm more concerned about streamer dependency than this temporary nodejs fs implementation, but that may not be
relevant in this pull request?

As pointed out in readme.md this is also temporary, we can either rewrite or integrate stuff from it. Again I really want to change integrate cfx-js as soon as possible, once it works we'll be able to do a cleanup without blocking any of the project goals. Also note that these dependencies
won't be shipped with FF.

@ghost ghost assigned mykmelez Sep 19, 2012
@ghost ghost assigned ochameau Oct 1, 2012
@Gozala
Copy link
Contributor Author

Gozala commented Oct 30, 2012

With this changes inn and clone of cfx-js under addon-sdk/cfx all the tests pass both on cfx-py and node. In other words this how you test it:

cd addon-sdk
git clone git://github.com/Gozala/cfx-js.git cfx
cd cfx

cfx test
Using binary at '/Applications/Firefox.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/0z/nztt5n4d4kg7b43dg9nv_3fc0000gp/T/tmpitLfnV.mozrunner'.
Running tests on Firefox 15.0/Gecko 15.0 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under darwin/x86.
..............................................................
62 of 62 tests passed.
Total time: 3.451745 seconds
Program terminated successfully.


npm test
Running all tests:
..............................................................
Passed:62 Failed:0 Errors:0

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