Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Commit

Permalink
Cleanup node module system code
Browse files Browse the repository at this point in the history
  • Loading branch information
@felixge @ry
felixge authored and ry committed Jan 12, 2011
1 parent 5f5201d commit 2e5dfaf
Showing 1 changed file with 170 additions and 153 deletions.

7 comments on commit 2e5dfaf

@felixge
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very sad to see this change: it totally reverses the idea of self-contained mini-require for natives and again includes it into the module system itself.

I don't see the problem with that. It feels a lot cleaner to me.

Any chance you take the requireNative, its cache (put it into requireNative.cache, if you want it accessible, it is on par with require.cache) and wrapping out?

Well, the main point of this refactoring was to get rid of this insane collection of locally scoped variables and functions that were really difficult to follow along. I wouldn't mind moving requireNative back outside, but I don't see the advantage, can you explain?

@ry
Copy link

@ry ry commented on 2e5dfaf Jan 13, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

felixge, "native" modules - the ones in lib/ are special in that they don't need the big require() function given to external modules. They can be bootstrapped with requireNative.

herby, sorry - we should have run this by you first.

@felixge
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

felixge, "native" modules - the ones in lib/ are special in that they don't need the big require() function given to external modules. They can be bootstrapped with requireNative.

I'm aware of that. I just don't get how this logic would not be part of the responsibility of the "module" module. Or at the very least it should be wrapped into a "NativeModule" class / module.

Again, my main problem was the fact that the "node.js" file was full of various scopes and local variables / functions, that would be referenced hundreds of lines apart.

By grouping those functions and variables into "class methods / properties", it becomes much easier to understand what the code is doing. If you want the "requireNative" functionality in it's own module, I can provide a patch for that.

@felixge
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, so if I provide a patch that pulls out requireNative into it's own unit, we're all happy? : )

@felixge
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is more of a political question, now: Is Module "hardwired, all-encompassing deeply integrated module system" or is it "a filebased module loader put on top of core requireNative thing"? I like the latter.

As of my last two comments, I'm still agreeing with you : ).

@felixge
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be if it won't need any code from Module to work (that is, it will work if I remove Module completely).

Sounds like a plan, I'll prepare a patch.

@felixge
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

herby, what do you think about this?

felixge@fa1c644

Please sign in to comment.