-
Notifications
You must be signed in to change notification settings - Fork 1
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
chore: restructure monorepo #231
Conversation
✅ Deploy Preview for sheepdog ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
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.
All looks good 🎉 I added one comment as a question more than anything, happy for you to merge without changing anything
@@ -9,7 +9,7 @@ const handler = (({ max = 1 }: { max?: number } = { max: 1 }) => { | |||
if (running_controllers.size >= max) { | |||
const first = running_controllers.values().next(); | |||
first.value?.abort(); | |||
running_controllers.delete(first.value); | |||
running_controllers.delete(first.value!); |
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.
so I don't understand why you needed to add this 🤔 the line above you're dealing with value possibly being null/undefined but here you're asserting that it's not? it seems like a real type change in a PR that is otherwise just moving things around
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 think the switcharoo that we did with the tsconfig caused the surface of this problem...but it's really a non problem because you can try to delete null
or undefined
and it will just not delete anything.
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.
yes but that should probably be reflected in the types no? what's the signature for delete?
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.
yes but that should probably be reflected in the types no? what's the signature for delete?
Nope, i think this is one of the rare instances when TS is more strict than it needs to be
interface Set<T> {
/**
* Appends a new element with a specified value to the end of the Set.
*/
add(value: T): this;
clear(): void;
/**
* Removes a specified value from the Set.
* @returns Returns true if an element in the Set existed and has been removed, or false if the element does not exist.
*/
delete(value: T): boolean;
/**
* Executes a provided function once per each value in the Set object, in insertion order.
*/
forEach(callbackfn: (value: T, value2: T, set: Set<T>) => void, thisArg?: any): void;
/**
* @returns a boolean indicating whether an element with the specified value exists in the Set or not.
*/
has(value: T): boolean;
/**
* @returns the number of (unique) elements in Set.
*/
readonly size: number;
}
we could make the Set
be Set<AbortController | undefined | null>
but tbf i'm perfectly fine with just asserting the existance.
Closes #229
The package
@sheepdog/core
is already published so everything should go smoothly (i hope 😅)