First steps in fixing #758 #767
Conversation
Signed-off-by: reluc <relu.cri@gmail.com>
Signed-off-by: reluc <relu.cri@gmail.com>
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- if cacheIdx is valid, then clients *contains* schemes[cacheIdx] | ||
| client = this.getClients().get(schemes[cacheIdx])!; |
There was a problem hiding this comment.
Would it make sense to throw an error here instead?
There was a problem hiding this comment.
The thing is that at the point we are sure that this.getClients().get(schemes[cacheIdx])! will not be undefined. Check line 508 cacheIdx will be not be -1 if this.getClients() contains that scheme.
| bytes = codec.valueToBytes(value, schema, par); | ||
| } else { | ||
| console.warn( | ||
| "[core/content-serdes]", | ||
| `ContentSerdes passthrough due to unsupported serialization format '${contentType}'` | ||
| ); | ||
| bytes = Buffer.from(value.toString()); | ||
| bytes = Buffer.from(value === null ? "" : value.toString()); |
There was a problem hiding this comment.
Is it intended to only check for null and not also for undefined (using value == null) here?
There was a problem hiding this comment.
So if we just trust the tsc compiler we know that value is not undefined, therefore it does not make sense. Some people say that in general is not guaranteed that people will use your library from TS and therefore you should take into account corner cases that are covered by the compiler. My problem here is that we don't have a policy. So if we check for undefined here, then we have to do it everywhere basically.... not sure if is something that we want.
There was a problem hiding this comment.
Hmm, there is a check above, though, which logs a warning if the value is undefined. To be consistent, that check would need to be removed as well?
My suggestion here would be to simply use the following:
| bytes = Buffer.from(value === null ? "" : value.toString()); | |
| bytes = Buffer.from(value?.toString() ?? ""); |
This way, the code is a bit more concise and undefined is also covered (which would otherwise cause an error, as toString() seems to not exist on undefined, in contrast to other data types).
There was a problem hiding this comment.
I might have agreed with @JKRhb's suggestion but then I thought more about the logic. IMHO is wrong... The else branch is activated if no codec is found and it defaults to simple sending value.toString or "". But value.toString() is not always correct. For example for objects it returns: [object Object] . Why should we send this to the wire? 🤔 So the resolve this conversation I would like to open an issue instead of fixing this detail. thumbs up if you agree
| @@ -239,7 +239,7 @@ export class ThingModelHelpers { | |||
| console.debug("[td-tools]", "http fetched:", parsedData); | |||
| resolve(parsedData); | |||
| } catch (e) { | |||
| console.error(e.message); | |||
| console.error("[td-tools]", (e as Error).message); | |||
There was a problem hiding this comment.
Maybe we could also call e.toString() here.
|
@JKRhb and @danielpeintner I'm starting working on this again. I'm not sure about what I need to address first. What do you prefer between:
|
|
I think we should focus on |
|
so you are ok even if the other packages have building errors? (my personal take is to go with this strategy) |
Signed-off-by: reluc <relu.cri@gmail.com>
One the short run either way is fine for me. What I think is a good way forward is to properly fix I don't know whether that makes sense for everybody? |
Ok then for a clear build I have to change also non-core packages... let's hope it is not a massive change! But to simplify the review process I would first fix @JKRhb's comments. |
see eclipse-thingweb#758 Signed-off-by: reluc <relu.cri@gmail.com>
Signed-off-by: reluc <relu.cri@gmail.com>
There was a problem hiding this comment.
Good work @relu91 👍
Looks good!
Since I don't have much time before the vacation and I think it would be good to merge it soon I tried to look as good as possible.
Note: The PR introduces some more // @ts-ignore we might want to look later.
BTW, see question below w.r.t. to killme.js
Signed-off-by: reluc <relu.cri@gmail.com>
|
File removed, merging. |
I've started the long journey in the pursuit of a type-safe code base. This PR is still a work in progress since I have still to solve the issues caused by my changes in the other packages. My plan in this PR is to have td-tools and core strictly checked. The good news is that they already are, BUT on the way, I had to make some choices that influenced the other packages. To fully close this PR I will fix the errors in the other packages to have the CI/CD work again. Meanwhile, I'd ask you to review core and td-tools because I'll try to not touch those anymore.
Let me know if you see something off! 👍🏻