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
fix: annotate ISC errors and split into to separate errors #1146
Conversation
β± Benchmark resultsComparing with b0048a4 largeDepsEsbuild: 7.7sβ¬οΈ 4.76% increase vs. b0048a4
LegendlargeDepsNft: 40.5sβ¬οΈ 4.38% increase vs. b0048a4
LegendlargeDepsZisi: 55sβ¬οΈ 1.36% increase vs. b0048a4
|
super(message) | ||
|
||
Object.setPrototypeOf(this, new.target.prototype) | ||
this.name = 'FunctionBundlingUserError' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines are necessary because typescript does not support extending builtins completely
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-2.html#support-for-newtarget
let scheduledFuncsFound = 0 | ||
const scheduledFunctionExpected = imports.some(({ imported }) => imported === 'schedule') | ||
let scheduledFunctionFound = false | ||
let scheduleFound = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the logic a little because all we care if schedule
was imported once, if someone imports multiple times, then that is fine too as long as one of the imports is used and exported.π€·ββοΈ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! β¨
Left a question that we might want to address before merging.
@@ -0,0 +1,70 @@ | |||
import type { Message } from '@netlify/esbuild' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to extract all the types from index.js
into types.js
, to avoid a cycle dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
β¦nd-ship-it into fix/danez/error-message
π Thanks for submitting a pull request! π
Summary
Fixes #1145
Annotates the errors and also splits them into to errors to guide users into the right direction.
For us to review and ship your PR efficiently, please perform the following steps:
This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing
a typo or something that`s on fire π₯ (e.g. incident related), you can skip this step.
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)