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 support - Revision to #137 and #126 #144

Closed
wants to merge 3 commits into from

Conversation

jdahlq
Copy link

@jdahlq jdahlq commented May 11, 2012

This is a revision of issues #137 and #126. I tested it on Windows and Mac OS X. Pull request #137 breaks Browserify for *nix because it replaces some of the *nix regexes with Windows-specific ones (e.g. instead of searching for a forward slash it searches for a backslash).

This new pull request removes most of the changes that seem unnecessary in #137. It changes only two areas of the lib/wrap.js code:

  1. For caching, it follows Update to make browserify work on Windows (recreated from #126, removed whitespace changes) #137 by using process.env.USERPROFILE when process.env.HOME is not available. I verified that this creates a ".config/browserify/cache.json" file in my Windows home directory.
  2. Backslashes are replaced by forward slashes in the makeTarget function. This is done at the beginning of the function so that most of the regexes to not need to be changed. Wherever path.normalize() is used, backslashes are again replaced with forward slashes.

Interested parties please test this code, especially because I removed quite a few of the previously proposed modifications in #137. I tested it on a project of moderate complexity.

Add support for Windows in a minimally intrusive way.
@cjblomqvist
Copy link

I'll check it out as soon as I can find the time! Thx for taking a further look at it!

@ddotlic
Copy link

ddotlic commented May 14, 2012

The patch works, but isn't still ideal yet.

Running the simplest test project from the repo (simple-build) using the repo version of browserify I got non-working output and ugly full paths in require.define for the foo and bar dependencies.

Using this patch I can get it to work, but looking at the output there is one minor annoyance: the file 'entry.js' is defined and required as "\entry.js" instead of "/entry.js". Please see below a couple example lines of browserify.js as produced when run on the simple-build example project:

require.define("/foo.js", ....
...
require.define("/bar.js", ....
...
require.define("\\entry.js", ...
...
require("\\entry.js");

Forgot to say: all this is on Windows (7 x64) which is my primary environment (have spare MacBook Air if you want me to test there too).

@cjblomqvist
Copy link

I believe I took care of this in the original pull request, so maybe something has been modified? Try with previous pull requests and see if they work and then try to track what has changed and what hasn't.

@jdahlq
Copy link
Author

jdahlq commented May 15, 2012

@ddotlic I believe my last commit addresses your concern. You are right, @cjblomqvist , your code had already fixed this cosmetic flaw. My initial pull request included only the lines of code that I understood to be necessary so that all of the changes are completely defensible.

I tested this again on Win7 and on OS X. Thanks to those who have tested; please test again with the new commit.

@ddotlic
Copy link

ddotlic commented May 16, 2012

@jdahlq Yes it does! Looks great, thanks. Tested on Win7 x64.

@domenic
Copy link
Contributor

domenic commented May 21, 2012

Any chance of getting this merged?

@domenic
Copy link
Contributor

domenic commented May 21, 2012

@jdahlq Since it looks like I'll be using your fork for the foreseeable future, would you mind please rebasing it on top of @substack's latest changes? :)

@jdahlq
Copy link
Author

jdahlq commented May 21, 2012

@domenic Done! I've never merged back into a fork before, but it looks like it worked (though I haven't tested it yet). I'll try to keep it up to date.

@domenic domenic mentioned this pull request Jun 5, 2012
@ghost ghost closed this Jun 5, 2012
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.

None yet

4 participants