Code review notes

[jdalton]

I wanted to capture the notes I took during our code review.

Moved

• An option to handle proposed syntax (nested import, namespace export) (moved to #100)
• Don't save setter for const exports. (moved to #134)

Completed

• Check if namespace import is constant and if so the bind isn't needed and it can be const.
• Use arrow functions for module.import and module.exports, module.import, and module.importSync.
• The namespace object should be Object.create(null)
• The cache hash - look into mtime + path + version.
• If a module has import/export add 'use strict'
• Align setEsModule __esModule property non-enumerable
• - Make the uniqueKey a fast property by changing the : to a __ or something similar
• Should addGetters throw? Yes.
• forEachSetter (use UNDEFINED lookup)

[jdalton]

Check if namespace import is constant and if so the bind isn't needed and it can be const.

The rules for namespace properties are here. The property descriptors should be:

{ [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: false }

Which means you can create the namespace object as an Object.create(null) and then pass it through Object.seal after you've assigned its properties. No need to have Object.defineProperty for all of them since Object.seal will take care of it.

[benjamn]

[[Writable]]: true though?

[benjamn]

I thought that to-do item was more to check if the namespace variable itself could be changed to a value different from the original object, which is what the <setter function>.bind(ns = Object.create(null)) protects against.

[jdalton]

[[Writable]]: true though?

That's what the spec says. I've pinged Brian Terlson to see if it's a spec bug.
Update: Ok so, it's still writable in the draft spec too.

I thought that to-do item was more to check if the namespace variable itself could be changed to a value different from the original object,

Ah ya. Imported identifiers are all const, even namespace ones, so the bind can be dropped and const used for namespace imports.

[jdalton]

I looked up taking on

Make the uniqueKey a fast property by changing the : to a __ or something similar

It looks like most all keys are the slow kind because they use file paths. This was a nice to have but not a biggie so crossing it out.