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

Fixing broken require: require('util') -> require('../utils') #395

Merged
merged 1 commit into from Mar 5, 2017

Conversation

Projects
None yet
2 participants
@ZackMFleischman
Contributor

ZackMFleischman commented Feb 6, 2017

Description

In the file lib/nodejs/NodejsStreamOutputAdapter.js, it trying to require util, but really wanted to pull in ../utils.

This likely fell through the cracks because the only place this file is required in the code, it's wrapped in a try/catch clause where the catch resolves to a no-op.

It's worth noting that this was causing my project to break when using webpack, as webpack tries to go in and follow all the require statements and bundle everything up, but it would hit this and break.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Feb 7, 2017

Collaborator

No, we really try to load util, provided by the nodejs runtime.
What's your webpack configuration ? It looks like you try to use nodejs streams while excluding global modules.

Collaborator

dduponchel commented Feb 7, 2017

No, we really try to load util, provided by the nodejs runtime.
What's your webpack configuration ? It looks like you try to use nodejs streams while excluding global modules.

@ZackMFleischman

This comment has been minimized.

Show comment
Hide comment
@ZackMFleischman

ZackMFleischman Feb 7, 2017

Contributor

@dduponchel Hey!

So apparently my webpack configuration was accidentally overwriting Node's util with our own 'util' alias and that was mucking up the works. I changed that and everything works hunky-dory.

Having said that though, I would still recommend merging this PR for the following reason:

NodejsStreamOutputAdapter.js is the only place in the whole repo where you pull in Node's util module directly. Every other module pulls in your own lib/utils.js module.

This module also has the inherits function (the only thing that NodejsStreamOutputAdapter.js actually uses from the module), and in fact, NodejsStreamOutputAdapter.js's sister file NodejsStreamInputAdapter.js does nearly precisely the same thing but uses lib/utils.js.

You can compare the code here:

// NodejsStreamOutputAdapter.js 
var util = require('util');
util.inherits(NodejsStreamOutputAdapter, Readable);

vs

// NodejsStreamInputAdapter.js 
var utils = require('../utils');
    ...
utils.inherits(NodejsStreamInputAdapter, GenericWorker);

Admittedly, it's a minor issue, and the code works the same regardless, but three cheers for code consistency? :)

Contributor

ZackMFleischman commented Feb 7, 2017

@dduponchel Hey!

So apparently my webpack configuration was accidentally overwriting Node's util with our own 'util' alias and that was mucking up the works. I changed that and everything works hunky-dory.

Having said that though, I would still recommend merging this PR for the following reason:

NodejsStreamOutputAdapter.js is the only place in the whole repo where you pull in Node's util module directly. Every other module pulls in your own lib/utils.js module.

This module also has the inherits function (the only thing that NodejsStreamOutputAdapter.js actually uses from the module), and in fact, NodejsStreamOutputAdapter.js's sister file NodejsStreamInputAdapter.js does nearly precisely the same thing but uses lib/utils.js.

You can compare the code here:

// NodejsStreamOutputAdapter.js 
var util = require('util');
util.inherits(NodejsStreamOutputAdapter, Readable);

vs

// NodejsStreamInputAdapter.js 
var utils = require('../utils');
    ...
utils.inherits(NodejsStreamInputAdapter, GenericWorker);

Admittedly, it's a minor issue, and the code works the same regardless, but three cheers for code consistency? :)

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Feb 15, 2017

Collaborator

I used util instead of ./utils because our inherits implementation is meant to be "good enough for our use cases". I feared edge cases were our implementation and nodejs' differ, leading to hard-to-debug issues. I also assumed util would always be available :-)
Both generate correct results in our tests on nodejs 0.10 -> 7.5, so that doesn't seem that risky.

What do you think of it ?

Collaborator

dduponchel commented Feb 15, 2017

I used util instead of ./utils because our inherits implementation is meant to be "good enough for our use cases". I feared edge cases were our implementation and nodejs' differ, leading to hard-to-debug issues. I also assumed util would always be available :-)
Both generate correct results in our tests on nodejs 0.10 -> 7.5, so that doesn't seem that risky.

What do you think of it ?

@ZackMFleischman

This comment has been minimized.

Show comment
Hide comment
@ZackMFleischman

ZackMFleischman Feb 15, 2017

Contributor

I think if you fear edge cases where your implementations differ then you should use Node's util.inherits everywhere in your codebase.

If you decide that your implementation is good enough for your use cases, then use your home rolled version everywhere.

Personally, your home rolled version is quite simple, and if something big changes, I don't imagine it will be as hard to debug as you imagine, and you could always switch over later. I think it's not particularly a big deal either way, but I think you should use 1 or the other throughout your codebase for consistencies sake.

Contributor

ZackMFleischman commented Feb 15, 2017

I think if you fear edge cases where your implementations differ then you should use Node's util.inherits everywhere in your codebase.

If you decide that your implementation is good enough for your use cases, then use your home rolled version everywhere.

Personally, your home rolled version is quite simple, and if something big changes, I don't imagine it will be as hard to debug as you imagine, and you could always switch over later. I think it's not particularly a big deal either way, but I think you should use 1 or the other throughout your codebase for consistencies sake.

@dduponchel

This comment has been minimized.

Show comment
Hide comment
@dduponchel

dduponchel Mar 5, 2017

Collaborator

@ZackMFleischman sorry, I though I merged this pull request two weeks ago...

Collaborator

dduponchel commented Mar 5, 2017

@ZackMFleischman sorry, I though I merged this pull request two weeks ago...

@dduponchel dduponchel merged commit ab3829a into Stuk:master Mar 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ZackMFleischman

This comment has been minimized.

Show comment
Hide comment
@ZackMFleischman

ZackMFleischman Mar 6, 2017

Contributor

Woot!

Contributor

ZackMFleischman commented Mar 6, 2017

Woot!

@dduponchel dduponchel referenced this pull request Aug 23, 2017

Merged

Release 3.1.4 #450

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment