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

Windows compatibility #158

Merged
11 commits merged into from
Jun 5, 2012
Merged

Windows compatibility #158

11 commits merged into from
Jun 5, 2012

Conversation

domenic
Copy link
Contributor

@domenic domenic commented Jun 5, 2012

This supersedes #113, #125, #126, #137, and #144.

82/83 tests pass. test/watch.js is the failure, with "Cannot find module './a'" I am not sure what this is about.

Please consider merging. I am happy to make any changes you desire to make it more merge-able.

domenic added 11 commits June 5, 2012 13:33
Before it was saying `EventEmitter.require` in the stack traces; now it says `Wrap.require`.
On Windows paths have backslashes, instead of forward slashes.
Previous substitutions had made them always IDs. If it's an absolute path, use `path.normalize` to revert them back to paths.
The existing logic was turning '/node_modules/jade/lib/jade.js' into 'C:\node_modules\jade\lib\jade.js', when '/node_modules/jade/lib/jade.js' was desired.

Fixes Jade tests.
.js files aren't valid Win32 applications. It should spawn `node`, with the .js file as an argument.
Have to use `fs.watch` instead of `fs.watchFile`. Tests still do not pass due to "Cannot find module './a'". So not sure if this was a good idea.
@ghost ghost merged commit b739766 into browserify:master Jun 5, 2012
@ghost
Copy link

ghost commented Jun 5, 2012

The failing watch test was just from treating fs.exists || path.exists like existsSync. All fixed and thanks for getting this running on windows!

@jdahlq
Copy link

jdahlq commented Jun 18, 2012

Thanks for all the hard work, @domenic!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants