Skip to content
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

thoughts #1

Closed
jamestalmage opened this issue Dec 5, 2015 · 10 comments
Closed

thoughts #1

jamestalmage opened this issue Dec 5, 2015 · 10 comments

Comments

@jamestalmage
Copy link
Member

@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
Copy link
Member Author

So... I think I ended up inadvertently solving istanbuljs/nyc#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
Copy link
Member Author

// @bcoe

@jamestalmage
Copy link
Member Author

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
Copy link
Member Author

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

@novemberborn
Copy link

@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
Copy link
Member Author

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 😄

@jamestalmage
Copy link
Member Author

@novemberborn
see 0f6da01

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

@novemberborn
Copy link

@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
Copy link
Member Author

Maybe. Created #2

@jamestalmage
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants