istanbuljs / append-transform

handle multiple require hooks
MIT License
21 stars 4 forks source link

thoughts #1

Closed jamestalmage closed 8 years ago

jamestalmage commented 8 years ago

@novemberborn

Take a look at this.

It's basically your nyc implementation with a few changes.

  1. Never uses Module.prototype, instead it does:

    // redacted - no longer true
  2. Uses Object.getPropertyDescriptor, and if it sees registered getter / setter, will forward updates to them.
jamestalmage commented 8 years ago

So... I think I ended up inadvertently solving https://github.com/bcoe/nyc/issues/70

Initially, this was going to be a listener that AVA used to watch how extensions installed behaved and decide whether or not to disable our test transforms (it still could be used for that).

Check out the test suite, specifically the last 4 tests.

jamestalmage commented 8 years ago

// @bcoe

jamestalmage commented 8 years ago

So, I am not sure what the heck to call this thing, or what it does.

Check out: this test.

It now realize what @novemberborn's clever hook really does, is ensure that nycs istanbul transform is applied last, so it can collect all the source-map information from downstream transforms. I didn't fully understand that when I dove into this.

jamestalmage commented 8 years ago

(note that in the test "wrapped" transforms are installed with this library, "conventional" would be one installed via something like pirates).

novemberborn commented 8 years ago

@jamestalmage this is nice! I like how it retains require.extensions as the API. Hooking into the lower level implementation makes it easier to retain compatibility with existing code (important for testing frameworks).

How does it deal with a new hook calling a previously stored reference to require.extensions['.js']? It probably wouldn't call _compile() in that case, meaning originalCompile is never restored.

jamestalmage commented 8 years ago

After installation, any reference to require.extensions['.js'] contains a reference to a wrapped hook instance. Any wrapped hook instance is going to immediately replace ._compile anyways, so I think it's fine, even if you skip it.

I'm like 60% sure of this :smile:

jamestalmage commented 8 years ago

@novemberborn see https://github.com/jamestalmage/require-extension-listener/commit/0f6da01e494b411087ca35e73f01216ef6b1659b

Do you think that covers all the scenarios you were thinking of?

novemberborn commented 8 years ago

@jamestalmage yea, quite extensively I must say.

I wonder if this should have a "revert" API like pirates? Right now if a hook decides to uninstall itself it also uncouples subsequent hooks.

jamestalmage commented 8 years ago

Maybe. Created #2

jamestalmage commented 8 years ago

Just published append-transform@0.2.0, thanks for helping me think this through.