refactor: handle also enhanced contentTypes

[danielpeintner]

e.g., expected "application/json" should deal with "application/json; charset=utf-8"

fixes #1250

[danielpeintner]

Question: is it enough to look into the contentType before ":"

Note1: I assume if we have response: { contentType: "application/json; charset=utf-8" } in a TD we expect to get exactly this encoding ... even though we could run into issues with multiple spaces after ";" etc. Not sure if we want to handle all this..

Note2: I assume we want to have the same check for properties/events as well, right?

[codecov]

Codecov Report

Attention: Patch coverage is 82.05128% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 77.84%. Comparing base (0b7b108) to head (df20650).

Files	Patch %	Lines
packages/core/src/consumed-thing.ts	82.05%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1251      +/-   ##
==========================================
+ Coverage   77.81%   77.84%   +0.02%     
==========================================
  Files          84       84              
  Lines       17523    17541      +18     
  Branches     1781     1783       +2     
==========================================
+ Hits        13635    13654      +19     
+ Misses       3853     3852       -1     
  Partials       35       35              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[egekorkan]

How about checking for string inclusion instead?

[danielpeintner]

How about checking for string inclusion instead?

I think to get it right we might want to use a dedicated library like

• https://www.npmjs.com/package/content-type-parser or
• https://www.npmjs.com/package/content-type

I fear that there might be some edge cases we miss otherwise...

[danielpeintner]

Note: I updated accordingly

• added the same InteractionOutput handling for readProperty also
• use dedicated library to parse contentType

[egekorkan]

I like the overall changes. Do we want to apply this to all interaction styles in this PR? In the long run, all should use this library.

[JKRhb]

Looks good to me in general :) See one relatively minor comment below:

[danielpeintner]

Do we want to apply this to all interaction styles in this PR?

01e0b84 adds the same handling for observeProperty and event

[JKRhb]

I am wondering if we could maybe also simplify these try-catch-blocks, as the handleInteractionOutput method itself is probably the only place where an error could originate from, right? But this would only be an optimization step.

[danielpeintner]

I was thinking about the same. In the first commits the try catch block was inside handleInteractionOutput().

The downside was/is that either the error message is not very clear (not stating for which interaction name & type it originates from) OR we need to pass additional arguments like interaction-type (property, action, event) and interaction-name is used to handleInteractionOutput() purely for logging.
Personally I didn't like that much and hence decided to do the error handling in each interaction even though I know it duplicates code.

Anyhow, I am open to either way...

[JKRhb]

The downside was/is that either the error message is not very clear (not stating for which interaction name & type it originates from) OR we need to pass additional arguments like interaction-type (property, action, event) and interaction-name is used to handleInteractionOutput() purely for logging.
Personally I didn't like that much and hence decided to do the error handling in each interaction even though I know it duplicates code.

Yeah, I agree, passing these parameters only for logging is definitely not that ideal. So I think we can keep it as is :) Maybe one potential solution for the future could be introducing a common interface for ConsumedThingProperty, ConsumedThingAction, ConsumedThinEvent and then passing the respective interaction affordance to the handler function which could then obtain the necessary information for logging.

[danielpeintner]

Is there anything else I can/should do?

[relu91]

In general, I'm a little bit reluctant to add external libraries to the core package unless they are really something needed. However, in this case, I can make an exception given the fact that the package is really just one single file. We can merge!