Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Change require.register signature and fix `require('toString')` #12

Merged
merged 5 commits into from Dec 30, 2012

Conversation

Projects
None yet
2 participants
Contributor

paulmillr commented Dec 22, 2012

  1. require.register sig was changed from module, exports, require to exports, require, module to match node.js and other implementations.
  2. Fixed bugs like require('toString'), require('hasOwnProperty').

In IE<=8 the bug will still exist, but more global check will require a lot more code. This will slightly encourage people to upgrade.

paulmillr added some commits Dec 22, 2012

Do `Object.create(null)` when it’s available.
This will get rid from stuff like `require('toString')`.

In IE<=8 the bug will still exist, but more global check will require
a lot more code. This will slightly encourage people to upgrade.

@paulmillr paulmillr referenced this pull request in componentjs/builder.js Dec 22, 2012

Closed

Change require.register signature. Closes gh-30. #52

Contributor

paulmillr commented Dec 22, 2012

needs tests, wip

Contributor

tj commented Dec 22, 2012

instead of going the Object.create(null) route we could just add a hasOwnProperty check

Contributor

paulmillr commented Dec 22, 2012

@visionmedia six checks

  return require.modules[reg] && reg
    || require.modules[regJSON] && regJSON
    || require.modules[index] && index
    || require.modules[indexJSON] && indexJSON
    || require.modules[orig] && orig
    || require.aliases[index];
Contributor

tj commented Dec 22, 2012

@paulmillr yeah true, probably not a big deal though since it does something like 100,000 ops/s at least and even a large app like ours probably only has ~700 requires at boot so a pretty small one time cost

Contributor

paulmillr commented Dec 24, 2012

@visionmedia changed object.null to has-prop, added tests etc.

Contributor

tj commented Dec 30, 2012

looks good but stylistic changes should be separate, I'll merge

tj added a commit that referenced this pull request Dec 30, 2012

Merge pull request #12 from paulmillr/topics/improve-compat
Change require.register signature and fix `require('toString')`

@tj tj merged commit 254ac04 into componentjs:master Dec 30, 2012

@paulmillr paulmillr deleted the paulmillr:topics/improve-compat branch Dec 30, 2012

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