refactor: redirect subsequent imports of "source-map-support" to this… #23

ejose19 · 2021-08-28T21:36:19Z

… package

EDIT from @cspotcode: blocked by #28 because I think we'll need to be sure the shared state also tracks installation of the redirections in this ticket

… package

cspotcode · 2021-08-28T21:45:35Z

For hooking node's CommonJS resolver, I think we need to use require('module')._resolveFilename. I'm pretty sure that's what tools like yarn PnP use.

What if someone does require(require.resolve('source-map-support')) or otherwise passes an absolute path? Can we make this logic more robust? Perhaps detecting when the package.json "name" is equal to source-map-support?

What if someone tries to import('source-map-support') or import from 'source-map-support'? I think we'll need to expose this functionality in a way that ts-node can implement redirection in its ESM loader, too.

Should this be opt-in via a flag? Or automatic? If the former, ts-node can easily pass the opt-in flag.

ejose19 · 2021-08-28T22:36:48Z

For hooking node's CommonJS resolver, I think we need to use require('module')._resolveFilename. I'm pretty sure that's what tools like yarn PnP use.

Done

What if someone does require(require.resolve('source-map-support')) or otherwise passes an absolute path? Can we make this logic more robust? Perhaps detecting when the package.json "name" is equal to source-map-support?

For 1st one, the latest change already detects it correctly, for 2nd one though, that seems like an very unusual case for importing source-map-support, is it worth the performance loss of deep inspecting each import just to cover that case? I'd say to keep things simple in order to not affect performance.

What if someone tries to import('source-map-support') or import from 'source-map-support'? I think we'll need to expose this functionality in a way that ts-node can implement redirection in its ESM loader, too.

Wouldn't that change the package exports interface? or isn't that an issue? Although for ESM I don't know how to overwrite the imports (maybe at compile/transpile level?)

Should this be opt-in via a flag? Or automatic? If the former, ts-node can easily pass the opt-in flag.

I'm fine with either, but given that doing nothing will cause source maps to be misaligned, I'd say to enable by default.

cspotcode

Although for ESM I don't know how to overwrite the imports (maybe at compile/transpile level?)

ts-node can do that within its ESM loader hooks, but only if our loader hook is used, which only happens if the user passes --loader. Instead, or in addition to the require.resolve redirection, perhaps we should detect competing source-map-support installations via Object.defineProperty setters when we install our hooks.

Do we need to consider if a library may use 'source-map-support''s API without calling install()? In those cases, redirecting the require.resolve is unnecessary.

source-map-support.js

ejose19 · 2021-08-29T21:50:59Z

Do we need to consider if a library may use 'source-map-support''s API without calling install()? In those cases, redirecting the require.resolve is unnecessary.

Actually, that's exactly the case with tslog, it's not using install but wrapCallSite fn only, so without a full redirect the correct line numbers won't be reported.

ts-node can do that within its ESM loader hooks, but only if our loader hook is used, which only happens if the user passes --loader. Instead, or in addition to the require.resolve redirection, perhaps we should detect competing source-map-support installations via Object.defineProperty setters when we install our hooks.

For this I don't know exactly what you suggest to apply, if it means the points you listed there: TypeStrong/ts-node#1441 (comment) then it will suffer the same issue as above, since the package could use other exports like wrapCallSite.

cspotcode · 2021-08-30T16:14:01Z

Actually, that's exactly the case with tslog, it's not using install but wrapCallSite fn only, so without a full redirect the correct line numbers won't be reported.

I see, I'm not sure what it's doing to get access to raw stack frames. Is it failing because it does not have access to our retrieveFile callback? So it is trying to find sourcemaps, but it cannot get access to the ones coming from ts-node?

https://github.com/TypeStrong/ts-node/blob/aaf60523ac0f77dc52b3c729f1f179a85dcac2c0/src/index.ts#L665-L679

cspotcode · 2021-08-30T16:29:50Z

For this I don't know exactly what you suggest to apply

I was thinking we can implement redirection from source-map-support to @cspotcode/source-map-support within our ESM resolve hook. I believe that ESM resolve ignores _resolveFilename since it's a separate codepath.

https://nodejs.org/dist/latest-v16.x/docs/api/esm.html#esm_resolve_specifier_context_defaultresolve
https://github.com/TypeStrong/ts-node/blob/aaf60523ac0f77dc52b3c729f1f179a85dcac2c0/src/esm.ts#L39-L73

ejose19 · 2021-08-30T17:56:43Z

I see, I'm not sure what it's doing to get access to raw stack frames. Is it failing because it does not have access to our retrieveFile callback? So it is trying to find sourcemaps, but it cannot get access to the ones coming from ts-node?

https://github.com/TypeStrong/ts-node/blob/aaf60523ac0f77dc52b3c729f1f179a85dcac2c0/src/index.ts#L665-L679

I've inspected the usage and wrapCallSite is first stored here to be later used here, it seems both this package and original package wrapCallSite are being called, I've placed a console.log (console.log('$pkgName', frame, state);) at the start of wrapCallSite and this is the result:

@cspotcode/source-map-support CallSite {} {
  nextPosition: {
    source: '/home/user/repos/test/b.ts',
    line: 3,
    column: 7,
    name: null
  },
  curPosition: {
    source: '/home/user/repos/test/b.ts',
    line: 3,
    column: 7,
    name: null
  }
}
@cspotcode/source-map-support CallSite {} {
  nextPosition: {
    source: '/home/user/repos/test/node_modules/tslog/src/LoggerWithoutCallSite.ts',
    line: 266,
    column: 27,
    name: null
  },
  curPosition: {
    source: '/home/user/repos/test/node_modules/tslog/src/LoggerWithoutCallSite.ts',
    line: 266,
    column: 27,
    name: null
  }
}
@cspotcode/source-map-support CallSite {} {
  nextPosition: {
    source: '/home/user/repos/test/node_modules/tslog/src/LoggerWithoutCallSite.ts',
    line: 333,
    column: 39,
    name: null
  },
  curPosition: {
    source: '/home/user/repos/test/node_modules/tslog/src/LoggerWithoutCallSite.ts',
    line: 333,
    column: 39,
    name: null
  }
}
@cspotcode/source-map-support CallSite {} {
  nextPosition: {
    source: '/home/user/repos/test/node_modules/tslog/src/LoggerWithoutCallSite.ts',
    line: 376,
    column: 54,
    name: null
  },
  curPosition: {
    source: '/home/user/repos/test/node_modules/tslog/src/LoggerWithoutCallSite.ts',
    line: 376,
    column: 54,
    name: null
  }
}
source-map-support CallSite {} undefined

I was thinking we can implement redirection from source-map-support to @cspotcode/source-map-support within our ESM resolve hook. I believe that ESM resolve ignores _resolveFilename since it's a separate codepath.

https://nodejs.org/dist/latest-v16.x/docs/api/esm.html#esm_resolve_specifier_context_defaultresolve
https://github.com/TypeStrong/ts-node/blob/aaf60523ac0f77dc52b3c729f1f179a85dcac2c0/src/esm.ts#L39-L73

So there's nothing to be done on this package for ESM support, but instead a PR on ts-node.

cspotcode · 2021-09-05T18:07:38Z

So there's nothing to be done on this package for ESM support, but instead a PR on ts-node.

Yeah, the only thing this package might want to do is expose a resolve function compatible with node's ESM resolve hook. Then ts-node could use that resolve function to handle redirection.

I'm reviewing this pull request today and might push some code changes to your branch.

ejose19 · 2021-09-28T15:40:41Z

Hello @cspotcode, were you able to check this PR? I'm using ts-node & tslog on many projects and the invalid stack traces surely makes debugging harder, I'd rather avoid installing a previous version of ts-node or monkey-patching tslog just to get it right, lmk if you need any other changes here.

cspotcode · 2021-09-28T16:03:14Z

I looked at it briefly but have been swamped with my paycheck job. I hope I'm able to make a ts-node release this weekend, and I can include these changes when I do that.

…

On Tue, Sep 28, 2021 at 11:40 AM ejose19 ***@***.***> wrote: Hello @cspotcode <https://github.com/cspotcode>, were you able to check this PR? I'm using ts-node & tslog on many projects and the invalid stack traces surely makes debugging harder, I'd rather avoid installing a previous version of ts-node or monkey-patching tslog just to get it right, lmk if you need any other changes here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC35OCTT4L427H2VJJCQY3UEHOYJANCNFSM5C7QMNGA> .

Allow installers to be notified upon redirection so they can log warnings to users Allow installers to opt-out of redirection Support both entrypoints: source-map-support and source-map-support/register

…Redirect

cspotcode · 2021-10-04T04:41:55Z

Made some progress on this. I had to fix and merge #28 first. Then I had to merge those changes into #29. Kind of annoying, but I want to get all those changes together for the next version since they kinda overlap.

I need to update this PR to store state on the sharedData struct introduced in #28. I also need to update uninstall() so that it removes the _resolveFilename hook.

cspotcode · 2021-10-04T04:44:41Z

Feel free to tackle the tasks listed in my previous comment, but if you don't have time, I will try to do them soon.

ejose19 · 2021-10-04T14:23:29Z

Regarding the failing test redirects require() of "source-map-support" to this module emits notifications previous to my last commit.

I see some issues with the test or the behavior you want to implement:

I've made so that next install don't set a new hook if one exists in sharedData (like you're doing for sharedData.errorPrepareStackTraceHook), but in that case, the test will fail if we don't uninstall & install first (since these onConflictingLibraryRedirect will never be "hooked")
If you really want for every subsequent install to override the previous one, then you should have put an extra require.resolve('source-map-support'); before both underTest.install to allow the first install to be called once.

So tell me what is the desired outcome (reuse already installed hook or always override) and I'll fix the test

cspotcode · 2021-10-04T20:04:39Z

Ideally we keep the single hook, but with an array of onConflictingLibraryRedirect functions to call.

So if there are 2x install() calls, then we install the hook only once, but we remember both onConflictingLibraryRedirect callbacks and call them both when we do a redirect.

…tall` of request redirect

ejose19 · 2021-10-04T20:46:09Z

Ok, should be good now. Only thing I don't like much is the jsdoc type for moduleResolveFilenameHook, although accurate it would be better to reuse what's already defined in the .d.ts file, but I suspect it's not possible (don't have experience working with jsdoc so that part will be up to you if you decide to change)

cspotcode · 2021-10-04T21:34:42Z

Thanks, I'll take a look soon. Agreed about the jsdoc. Ideally, you're supposed to use tsc to generate the .d.ts from the source code. I haven't had time to do that yet, and I've only been adding JSDoc to the .js insofar as it helps me avoid mistakes.

cspotcode

One more small change required; otherwise looks good. Thanks!

source-map-support.js

…alled on top if it

cspotcode

I had to tweak the enabled stuff but now looks good to go.

cspotcode · 2021-10-11T18:13:45Z

@ejose19 I just published ts-node 10.3.0 which includes this fix. Thanks again for fixing this, and if you hit any more issues please let me know.

https://github.com/TypeStrong/ts-node/releases/tag/v10.3.0

ejose19 · 2021-10-11T21:10:47Z

@cspotcode I confirm it's working correctly now, thanks!

pe8ter · 2021-10-27T17:52:34Z

This is a breaking change.

I have an Angular CLI project that uses Karma to run tests. I wrote my Karma config in TypeScript, so Karma uses ts-node under the hood to transpile the config. Since the Angular CLI uses karma-source-map-support to load source-map-support, these changes break my tests because the symbol sourceMapSupport isn't defined anymore in the browser.

For reference, the changes in this PR affect resolving the path to the source-map-support package, and later, constructing a path to the actual file, which is named differently in the two packages. Patching the names doesn't work anyway because the version in this repo is incompatible ("require is not defined" error).

Not sure what to do. Is there any way to flex here, or should I pester Google? I don't know if anyone maintains karma-source-map-support anymore.

ejose19 · 2021-10-27T18:08:50Z

Besides checking if you can just use this module instead of karma-source-map-support (since it contains many improvements over the original source-map-support), I think this can be fixed in ts-node side by just making this:
https://github.com/TypeStrong/ts-node/blob/bc03a3e18d04197f61f9e91befdc294590758077/src/index.ts#L723 be configurable through an option, although you may stumble with other issues related to TypeStrong/ts-node#1441

cspotcode · 2021-10-27T18:37:12Z

We could also add an option to disable ts-node's registration of source-map-support entirely. ts-node --install-source-map-support=false or "installSourceMapSupport": false perhaps.

Related tickets, though each is slightly different:

TypeStrong/ts-node#796 talks about turning off sourcemaps entirely, whereas I'm describing a feature that continues to generate sourcemaps but does not register source-map-support

TypeStrong/ts-node#1473 talks about automatically skipping source-map-support installation when the user has node's --enable-source-maps turned on

cspotcode · 2021-10-27T18:42:42Z

I've created a ticket for the idea I've described above: TypeStrong/ts-node#1533
It is labelled appropriately so that we will accept a pull request implementing it.

Pull requests and constructive criticism are welcome.

refactor: redirect subsequent imports of "source-map-support" to this…

ce6eafe

… package

refactor: change implementation to hook on Module._resolveFilename

48d7b0a

refactor: list explicit arguments

1ebbd86

cspotcode requested changes Aug 29, 2021

View reviewed changes

source-map-support.js Outdated Show resolved Hide resolved

refactor: return full resolved path for this package proxy

eb81879

cspotcode added 4 commits October 3, 2021 23:56

Several changes:

44c3bce

Allow installers to be notified upon redirection so they can log warnings to users Allow installers to opt-out of redirection Support both entrypoints: source-map-support and source-map-support/register

Merge remote-tracking branch 'origin/master' into ej/sourceMapSupport…

94e5587

…Redirect

Correct types; add tests

e865d19

Merge remote-tracking branch 'origin/master' into ej/sourceMapSupport…

8ab8b7a

…Redirect

refactor: adjust install & uninstall to use sharedData

f5db819

refactor: add onConflictingLibraryRedirect array to notify all `ins…

226b96a

…tall` of request redirect

cspotcode requested changes Oct 5, 2021

View reviewed changes

source-map-support.js Show resolved Hide resolved

refactor: only apply redirect if the hook is enabled

a0fdd9b

ejose19 requested a review from cspotcode October 5, 2021 19:19

ensure disabled hook fn remains disabled when subsequent hook is inst…

a4e1648

…alled on top if it

cspotcode approved these changes Oct 5, 2021

View reviewed changes

cspotcode merged commit c7db746 into cspotcode:master Oct 6, 2021

cspotcode mentioned this pull request Oct 27, 2021

Add flag to skip source-map-support install() TypeStrong/ts-node#1533

Open

pe8ter mentioned this pull request Oct 27, 2021

node_modules/@cspotcode/source-map-support/browser-source-map-support.js does not match any file. tschaub/karma-source-map-support#30

Closed

This was referenced Feb 6, 2022

Missing source-map-support dependency, fails with Yarn Berry PnP #35

Closed

[Bug]: jest fails when @cspot/source-map-support and yarn 2+ pnp are involved. jestjs/jest#12366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: redirect subsequent imports of "source-map-support" to this… #23

refactor: redirect subsequent imports of "source-map-support" to this… #23

ejose19 commented Aug 28, 2021 •

edited by cspotcode

Loading

cspotcode commented Aug 28, 2021

ejose19 commented Aug 28, 2021

cspotcode left a comment

ejose19 commented Aug 29, 2021

cspotcode commented Aug 30, 2021

cspotcode commented Aug 30, 2021

ejose19 commented Aug 30, 2021

cspotcode commented Sep 5, 2021

ejose19 commented Sep 28, 2021

cspotcode commented Sep 28, 2021 via email

cspotcode commented Oct 4, 2021

cspotcode commented Oct 4, 2021

ejose19 commented Oct 4, 2021

cspotcode commented Oct 4, 2021

ejose19 commented Oct 4, 2021

cspotcode commented Oct 4, 2021

cspotcode left a comment

cspotcode left a comment

cspotcode commented Oct 11, 2021

ejose19 commented Oct 11, 2021

pe8ter commented Oct 27, 2021

ejose19 commented Oct 27, 2021 •

edited

Loading

cspotcode commented Oct 27, 2021

cspotcode commented Oct 27, 2021

refactor: redirect subsequent imports of "source-map-support" to this… #23

refactor: redirect subsequent imports of "source-map-support" to this… #23

Conversation

ejose19 commented Aug 28, 2021 • edited by cspotcode Loading

cspotcode commented Aug 28, 2021

ejose19 commented Aug 28, 2021

cspotcode left a comment

Choose a reason for hiding this comment

ejose19 commented Aug 29, 2021

cspotcode commented Aug 30, 2021

cspotcode commented Aug 30, 2021

ejose19 commented Aug 30, 2021

cspotcode commented Sep 5, 2021

ejose19 commented Sep 28, 2021

cspotcode commented Sep 28, 2021 via email

cspotcode commented Oct 4, 2021

cspotcode commented Oct 4, 2021

ejose19 commented Oct 4, 2021

cspotcode commented Oct 4, 2021

ejose19 commented Oct 4, 2021

cspotcode commented Oct 4, 2021

cspotcode left a comment

Choose a reason for hiding this comment

cspotcode left a comment

Choose a reason for hiding this comment

cspotcode commented Oct 11, 2021

ejose19 commented Oct 11, 2021

pe8ter commented Oct 27, 2021

ejose19 commented Oct 27, 2021 • edited Loading

cspotcode commented Oct 27, 2021

cspotcode commented Oct 27, 2021

ejose19 commented Aug 28, 2021 •

edited by cspotcode

Loading

ejose19 commented Oct 27, 2021 •

edited

Loading