-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat(GridEngine): charAdded subject and public characterShifted obs #268
Conversation
I wasn't sure whether I should modify your existing charRemoved subject and instead opted for adding a new one for the addition of new characters. Also naming wise I was going against |
src/GridEngine.ts
Outdated
/** | ||
* @returns Observable that emits when a new character is added or an existing is removed. | ||
*/ | ||
characterShifted(): Observable<{ | ||
charId: string; | ||
action: "REMOVED" | "ADDED"; | ||
}> { | ||
return this.charAdded$.pipe( | ||
map<string, { charId: string; action: "REMOVED" | "ADDED" }>((c) => ({ | ||
charId: c, | ||
action: "ADDED", | ||
})), | ||
mergeWith( | ||
this.charRemoved$.pipe( | ||
map<string, { charId: string; action: "REMOVED" | "ADDED" }>((c) => ({ | ||
charId: c, | ||
action: "REMOVED", | ||
})) | ||
) | ||
) | ||
); | ||
} |
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.
How about introducing an enum and an interface? That makes it more readable and in case of a change you don't need to touch 3 type declarations:
/** | |
* @returns Observable that emits when a new character is added or an existing is removed. | |
*/ | |
characterShifted(): Observable<{ | |
charId: string; | |
action: "REMOVED" | "ADDED"; | |
}> { | |
return this.charAdded$.pipe( | |
map<string, { charId: string; action: "REMOVED" | "ADDED" }>((c) => ({ | |
charId: c, | |
action: "ADDED", | |
})), | |
mergeWith( | |
this.charRemoved$.pipe( | |
map<string, { charId: string; action: "REMOVED" | "ADDED" }>((c) => ({ | |
charId: c, | |
action: "REMOVED", | |
})) | |
) | |
) | |
); | |
} | |
/** Some docs here */ | |
export interface CharacterShift { | |
charId: string; | |
action: CharacterShiftAction; | |
} | |
/** Some docs here */ | |
export enum CharacterShiftAction { | |
REMOVED = "REMOVED", | |
ADDED = "ADDED", | |
} | |
/** | |
* @returns Observable that emits when a new character is added or an existing is removed. | |
*/ | |
characterShifted(): Observable<CharacterShift> { | |
return this.charAdded$.pipe( | |
map((charId) => ({ | |
charId, | |
action: CharacterShiftAction.ADDED, | |
})), | |
mergeWith( | |
this.charRemoved$.pipe( | |
map((charId) => ({ | |
charId, | |
action: CharacterShiftAction.REMOVED, | |
})) | |
) | |
) | |
); | |
} |
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.
Could you also update https://github.com/Annoraaq/grid-engine/blob/master/src/main-esm.ts with the new exports?
i'm not sure those are actually needed cause they should in theory be handled by the default export. I'll try the branch out tomorrow and if it doesnt work I'll add em. 👍 |
yep I totally forgot CharacterShiftAction became a string enum, which is compiled to a dictionary, and as such needs dedicated re-exports in contrast to the previous union string type. I guess you're preferring string-enums for JS consumers, so they have a valid reason in your lib. So can it be generally said that string-enums should be used whenever a limited set of "strings" is valid for a parameter? What I really do wonder though is if there is a way to automate, or at least warn, if a new string enum is introduced, that it should be exported in main-esm as well. Going for a custom ts-eslint rule sounds like quite an overkill :) |
I do prefer enums over union string types because you can iterate over them and also refactoring is easier if you want to rename something or find all usages. For JS consumers it should not matter because they have no static type safety anyway. I mean they theoretically COULD use an exported enum, because it gets transpiled into a JS object, but that won't help them much. I think I didn't understand the part that string enums need automatic export and union types do not. Do you refer only to JS users? The reason I had in mind for exporting is that TS users can use the enums. For example when calling the |
if you use Direction.Left its the same as As for iterability,
brings the same benefits of dynamically constructed string unions. Pure TS types that transpile into nothing, do not need to be explicitely exported in the esm bundle as they are available in the d.ts file and would result inside the ESM, well in nothing ;) String enums, aka Objects after transpile on the other hand need to be there Edit: refactoring with string unions/literals is also possible microsoft/TypeScript#5602 |
Yes it is a dictionary eventually at runtime. If you import it you can also use it as a type for a TS consumer:
My assumption (for some reason that I don't remember anymore) was that it was not possible to use it if it is not explicitly exported in the esm file (independent of whether it is an enum or let's say an interface). But when I was testing it before, both were correctly exported (enum and other types). But maybe my test was screwed up? So if we don't export it in main-esm.ts, would there be a consequence for TS users or only for JS users? Enums vs union typesThe const assertion approach you suggest works for iteration. However, you have to define 2 things. And also you have to import 2 things (one for iteration and one for typing). That always felt clumpsy to me. Considering the refactoring: When I have an example code like this: const directions = [ "Left", "Right"] as const;
type Direction = typeof directions[number];
const differentTypes = [ "Left", "Loft"] as const;
type DifferentType = typeof differentTypes[number];
const testDir: Direction = "Left";
const test: DifferentType = "Left"; And now I want to rename With VSCode I can't go and use the |
i cant see a reason why an interface would need to be additionally explicitely exported in the esm. I can access the type info and even do a type-import although there is no explicit definition. same applies to string literal types. Enum, both numeric and strings, on the other hand get transpiled and are needed, which I've replicated in my game, hence added the esm export as you mentioned but only for the string enum not So if we dont export string enums, there are consequences for both consumers (TS, JS) in that they can only use pure strings as for refactoring. tbh no idea, I'd have to give it a try. I typically only extend enums and rarely modify them, which is why the |
Alright I've tried it (with VSCode) and the following is happening for refactor-rename: // REFACTOR-RENAME LEFT doesn't work
const foo = ["LEFT", "LEFT2"] as const;
type Foos = typeof foo[number];
function test(a: Foos) {
console.log(a);
}
test("LEFT");
// REFACTOR RENAME on A -> X works and the function call at the bottom is changed to X as well
type A = "A" | "B";
function test2(a: A) {
console.log(a);
}
test2("A"); |
Ok I have tested it again. I think I now understood how it works and that is probably what you were trying to tell me. const dir: Direction = gridEngine.getFacingDirection(); However, you could not do const dir: Direction = Direction.LEFT; It makes sense because That means that I can remove all the interfaces from the esm file. So I think when it comes to the question whether we should use enums or union types (with const assertions) the pros and cons are mainly refactoring vs explicit exports. (For the refactoring I assume it means in VSCode so far it works for union types but not for const assertions yet.) We have to stick with enums until the next major release anyway to stay backward compatible. And since the enums are already used in the codebase I think it is easiest to just stick with them for now. To tackle the explicit export problem I think I found a solution. We could just use |
Exactly
Yep
From the consumer perspective it's whether you can assign just a string in type-safe way (string unions) or need to import the string enum and then use it. So the import statement is the difference for the consumer.
Whichever way tbh is totally fine for me. I've just started this conversation so that I know how to proceed for future PRs, so I'll stick to string enums as well.
Yep, that works. At least as long as everything is re-exported from |
Btw, sorry I've started this convo in the PR, definitely not the best place for it ;) Where would you see questions like those fit? A new issue? A GitHub discussion? |
Oh yes. Good point. I think with version 3 of GridEngine we should reconsider if using enums is still a good choice.
No worries. Where to place it depends a bit on whether it is a pure question or a suggestion/concern. At least that is my opinion. However, I don't think it is a big difference. |
Adds an observable emitting when either a new character was added or an existing deleted
Related issue #265