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

Support multiple instances of source-map-support #28

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

cspotcode
Copy link
Owner

Fixes #26
Fixes evanw/node-source-map-support/issues/91

@rhendric would you mind giving this a code review to make sure I didn't break anything? I cherry-picked your pull request and resolved merge conflicts. I had to tweak evanw/node-source-map-support/pull/215 a bit to be compatible, but it ended up simpler overall.


original issue description from @rhendric

This covers the use cases that b40cc25 doesn't; namely, if several
libraries reference different versions of source-map-support, then they
would previously all get their own source map caches, handler arrays,
etc., and not work with each other. This commit moves such data from
module-local variables into a versioned global object, enabling multiple
instances of source-map-support to cooperate.

This covers the use cases that b40cc25 doesn't; namely, if several
libraries reference different versions of source-map-support, then they
would previously all get their own source map caches, handler arrays,
etc., and not work with each other. This commit moves such data from
module-local variables into a versioned global object, enabling multiple
instances of source-map-support to cooperate.
@cspotcode
Copy link
Owner Author

Should we move this __sourceMapSupport field onto the shared state, too?
https://github.com/cspotcode/node-source-map-support/blob/master/source-map-support.js#L584

@rhendric
Copy link
Contributor

rhendric commented Sep 6, 2021

Should we move this __sourceMapSupport field onto the shared state, too?
https://github.com/cspotcode/node-source-map-support/blob/master/source-map-support.js#L584

I don't think that's necessary; it's already being assigned to something that's global.

This looks fine to me, although I haven't mucked about in the internals of this library since approximately when I wrote that PR originally, so I may not be the best person to catch any issues.

@cspotcode
Copy link
Owner Author

Thanks, it's tough since source-map-support really hasn't been touched in forever, as far as I can tell. All the PRs and issues are pretty old.

The concern I have with __sourceMapSupport is that Module.prototype._compile might be wrapped by other libraries, not just source-map-support. If another library wraps it and does not copy the __sourceMapSupport field, then it will appear as though source-map-support's hook is not installed even though it is, because it's been wrapped by another function.

@rhendric
Copy link
Contributor

rhendric commented Sep 6, 2021

I figure that's either intentional or an orthogonal flaw. Even if only one instance of source-map-support is loaded, something else wrapping the compile function after source-map-support is activated would have hidden that field. It would have been easy for that code to have just used a var in the module's scope instead of assigning a property on the compile method; although I haven't thought deeply about it, I can imagine that this was the intended behavior, in which case adding support for multiple instances shouldn't affect it. (Maybe run-time transpilers that wrap the compile method change the filename on the inner call and the caches should then have two entries?)

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

Successfully merging this pull request may close these issues.

Unable to create multiple instances Support multiple instances of source-map-support
2 participants