[core-http] enable no-prototype-builtins #12717
Conversation
maorleger
requested review from
bterlson,
chradek,
daviwil,
jeremymeng,
willmtemple and
xirzec
as code owners
ghost
added
sdk/core/core-http/package.json
Outdated
| @@ -82,7 +82,7 @@ | |||
| "integration-test:node": "echo skipped", | |||
| "integration-test": "npm run integration-test:node && npm run integration-test:browser", | |||
| "lint:fix": "eslint package.json api-extractor.json src test --ext .ts --fix --fix-type [problem,suggestion]", | |||
| "lint": "eslint package.json api-extractor.json src test --ext .ts -f html -o core-http-lintReport.html || exit 0", | |||
| "lint": "eslint package.json api-extractor.json src test --ext .ts -f html -o core-http-lintReport.html", | |||
There was a problem hiding this comment.
Feel free to let me know if I should revert this - it was easier for me to see the lint failure until I cleaned up the errors and then I figured it might be good to prevent future errors?
There was a problem hiding this comment.
I think this was done to prevent lint from failing CI at one point, though maybe we don't need it anymore? /cc @willmtemple
| @@ -49,8 +49,7 @@ class FetchHttpMock implements HttpMockFacade { | |||
|
|
|||
| // returns the locally mocked fetch instance | |||
| getFetch(): typeof node_fetch { | |||
| /// @ts-ignore | |||
| return this._fetch as typeof node_fetch; | |||
| return (this._fetch as unknown) as typeof node_fetch; | |||
There was a problem hiding this comment.
Removing @ts-ignore exposed this error. I just used the editor's Code Action to auto-fix it but it seems to line up with https://microsoft.github.io/TypeScript-New-Handbook/everything/#type-assertions - maybe there's a cleaner way by updating FetchMockSandbox? I wasn't sure...
There was a problem hiding this comment.
I think this one might be from when @jeremymeng did #9880
I'm fine with your solution though, casting through unknown is closer to the truth than us ignoring the line altogether.
There was a problem hiding this comment.
Looks good! I spent some time trying to make the mocked type compatible with type of node_fetch but it seemed non-trivial and at the end I chose to do cast instead for testing.
sdk/core/core-http/package.json
Outdated
| @@ -82,7 +82,7 @@ | |||
| "integration-test:node": "echo skipped", | |||
| "integration-test": "npm run integration-test:node && npm run integration-test:browser", | |||
| "lint:fix": "eslint package.json api-extractor.json src test --ext .ts --fix --fix-type [problem,suggestion]", | |||
| "lint": "eslint package.json api-extractor.json src test --ext .ts -f html -o core-http-lintReport.html || exit 0", | |||
| "lint": "eslint package.json api-extractor.json src test --ext .ts -f html -o core-http-lintReport.html", | |||
There was a problem hiding this comment.
I think this was done to prevent lint from failing CI at one point, though maybe we don't need it anymore? /cc @willmtemple
| @@ -49,8 +49,7 @@ class FetchHttpMock implements HttpMockFacade { | |||
|
|
|||
| // returns the locally mocked fetch instance | |||
| getFetch(): typeof node_fetch { | |||
| /// @ts-ignore | |||
| return this._fetch as typeof node_fetch; | |||
| return (this._fetch as unknown) as typeof node_fetch; | |||
There was a problem hiding this comment.
I think this one might be from when @jeremymeng did #9880
I'm fine with your solution though, casting through unknown is closer to the truth than us ignoring the line altogether.
maorleger
changed the title
maorleger
changed the title
Co-authored-by: Deyaaeldeen Almahallawi <dealmaha@microsoft.com>
What
Why
Fixing the no-prototype rule was punted to a future commit to limit scope
In passing, I noticed other errors in the lint report
When all were fixed, I figured it would be helpful to fail with an error so the user can tell where to find the lint report. Otherwise I had to figure out why the lint errors weren't showing up like they do in other projects
Ran unit tests
Ensure the lint report is clean of errors (but not warnings to limit changes)
Fixes #8972