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 fs.readFileSync instead of require.resolve #2797

Merged
merged 2 commits into from
Jan 22, 2017

Conversation

a-lucas
Copy link
Contributor

@a-lucas a-lucas commented Dec 30, 2016

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

Browserify doesn't supports require.resolve, and as a consequence, makes nexe fails the compilation :

New behaviour

This PR attempts to read the content of the socket.io-client file via fs.readFileSync and falls back to the original require.resolve if this file cannot be found.

Other information (e.g. related issues)

nexe/nexe#289

@darrachequesne
Copy link
Member

Hi, thanks for the PR! How about using the following:

function resolvePath(file){
  var filepath = path.resolve(__dirname, './../../', file);
  if (exists(filepath)) {
    return filepath;
  }
  return require.resolve(file);
}

Can you confirm the client file gets properly served that way? (you can also disable serving the client file with serveClient: false)

@darrachequesne
Copy link
Member

Else, brfs may be helpful here, if you actually want to serve the file.

@a-lucas
Copy link
Contributor Author

a-lucas commented Jan 21, 2017

@darrachequesne I never heard of brfs before, I also reckon your first suggestion is the best one, much DRYer, and also it doesn't requires an additional dependency.

Yes it works, nexe works great with socket.io with this change.

@a-lucas a-lucas force-pushed the master branch 2 times, most recently from c469daf to 63c28c0 Compare January 21, 2017 21:29
@darrachequesne darrachequesne merged commit 3b5f433 into socketio:master Jan 22, 2017
@darrachequesne
Copy link
Member

Thanks!

@a-lucas
Copy link
Contributor Author

a-lucas commented Jan 22, 2017

Thanks too! Socket.io is great!

@darrachequesne darrachequesne added this to the 2.0.0 milestone May 13, 2017
@khuongduybui
Copy link

Has this been released? I'm using socket.io 2.0.4 and is still plagued with "require.resolve" calls.

@Bolandish
Copy link

I'm also getting this error..

dzad pushed a commit to dzad/socket.io that referenced this pull request May 29, 2023
…ocketio#2797)

Browserify doesn't support require.resolve, and as a consequence, makes nexe fail the compilation. This PR attempts to get the path of the socket.io-client file via path.resolve and falls back to the original require.resolve if this file cannot be found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants