feat: drop internal functions from Deprecation Error stack trace #3426
Conversation
JSII can throw Deprecation Error and fail compilation when a deprecated property, enum or method is being used when `JSII_DEPRECATED` environment variable is set to `fail`. The deprecation error is throw from `.warnings.jsii.js` generated file. When nested prop of enum is deprecated, the stack trace will contain lot of methods from `.warnings.jsii.js` and since the file is minimized, tools like Jest which try to be helpful by printing the line where the error is thrown shows a wall of text without much useful information. With this change, the Deprecation error is caught and rethrown from top level class which call the validator to provide a clean stack trace. This approch is taken instead of passing the call site to deprecation handler as we can't get call site for setters and getters that can be used by `Error.captureStack`
RomainMuller
force-pushed
the
jsii-deprecation-error-code
branch
from
0888e83 to
41d2fe9
Compare
| try { | ||
| vm.runInContext(source, context, { filename: 'index.js' }); | ||
| // The above line should have resulted in a DeprecationError being thrown | ||
| expect(null).toBeInstanceOf(Error); |
There was a problem hiding this comment.
Not sure if I understand this line? Is the intention here to intentionally fail the test? Maybe something like this could make it clearer:
| expect(null).toBeInstanceOf(Error); | |
| throw new Error("expected a DeprecationError from running the transliterated snippet") |
Or maybe "expect(true).toEqual(false)"
There was a problem hiding this comment.
The expect(null) check guarantees that the test will be marked as failed if no exception was thrown. If we move to the throw style, it'll trigger the catch and will happily update the snapshot when running with -u, which I think is not desirable behavior.
I kind of agree with your sentiment here, but this "hack" is just a bit safer.
Ideally - I'd have wanted to do expect(() => vm.runInContext(...)).toThrow, but that'll only verify the error message, not the stack trace (which is what I care about here).
| // this emulates the situation that would be if the file had been through a | ||
| // bundler by turning all sequences of white spaces (new line included) into | ||
| // single spaces. | ||
| require: (id: string) => { |
There was a problem hiding this comment.
That's the sort of dance you have to do when you use VM contexts in node. The context keys are global values that'll be available within the sandbox; which otherwise only has access to ES primordials... So you need to give it "a" require function, and that function is a pretty standard implementation here...
All those APIs are stable enough that they won't break under us when we upgrade to a new node version, too.
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
|
Merging (with squash)... |
JSII can throw Deprecation Error and fail compilation when a deprecated property,
enum or method is being used when
JSII_DEPRECATEDenvironment variable is setto
fail. The deprecation error is throw from.warnings.jsii.jsgenerated file.When nested prop of enum is deprecated, the stack trace will contain lot of methods
from
.warnings.jsii.jsand since the file is minimized, tools like Jest which try to behelpful by printing the line where the error is thrown shows a wall of text without much useful information.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.