Skip to content
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

Update @context URLs, add response contentType workaround #52

Merged
merged 5 commits into from
May 23, 2023

Conversation

JKRhb
Copy link
Contributor

@JKRhb JKRhb commented Apr 23, 2023

This PR updates the @context URLs included in the directory's TM/TD in anticipation of w3c/wot-discovery#471 and applies the workaround added in w3c/wot-discovery#469 to the ThingDescriptionBuilderService by inserting application/x-empty as a contentType for response objects where the field absent.

Combined, these changes result in a TD for the Directory that passes validation against the latest JSON Schema document provided in the TD repository :)

@JKRhb JKRhb force-pushed the contentType-fix branch 2 times, most recently from f318c6a to 8f0b128 Compare April 23, 2023 10:09
@JKRhb
Copy link
Contributor Author

JKRhb commented Apr 23, 2023

Needed to update two dependencies and create another workaround in 8f0b128 to get the TD instantiation to actually work again. Seems to me like a bug has appeared in the td-tools package that modifies the input TM, which apparently has not happened before 🤔

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jan! As always thank you really much for contributing to Zion. The changes look good, but I checked the difference in package-lock and saw changes in unrelated packages like Fastify. Do you have an idea why this is happening? Can you try to narrow down the changes to only node-wot and wot-thing-description-types packages?

@JKRhb
Copy link
Contributor Author

JKRhb commented Apr 28, 2023

Hi Cristiano, thank you for the feedback! Sure, I will check again and try to narrow down the changes :)

@JKRhb
Copy link
Contributor Author

JKRhb commented Apr 28, 2023

With the latest push, I managed to trim down the changes in the package-lock.json file by a lot. However, for some reason, the tests are now failing since the td-tools apparently does not include the latest fixes for the TM JSON schema 🤔 We may need to put this PR on hold then until version 0.8.6 of the td-tools package with the updated schema has been released.

@relu91
Copy link
Member

relu91 commented May 3, 2023

Ok, in my understanding we are going to release 0.8.6 very soon. I'll check if we get the green light once it will be published!

@JKRhb
Copy link
Contributor Author

JKRhb commented May 5, 2023

The new td-tools release actually solved the problem 🎉

@JKRhb
Copy link
Contributor Author

JKRhb commented May 5, 2023

I am now wondering: Should we wait for w3c/wot-discovery#471? Or would it be fine to already merge this PR?

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ok for merging this right now, I'm confident that the linked PR in the discovery repo will be merged soon.

@relu91 relu91 requested a review from hyperloris May 16, 2023 07:31
@JKRhb
Copy link
Contributor Author

JKRhb commented May 23, 2023

w3c/wot-discovery#471 has now been merged by the way, so the spec is not a potential blocker anymore :)

@relu91
Copy link
Member

relu91 commented May 23, 2023

Talked offline with @hyperloris, he was a little bit uncomfortable with merging a PR with TODOs in it. I agree but those TODOs are reasonable and they can be tracked in issues (as suggested by @hyperloris himself). Therefore, I'm forcing the merging of the PR without waiting for his formal approval. Later on, I'll open the tracking issues.

@relu91 relu91 merged commit cc5a16a into vaimee:main May 23, 2023
@JKRhb JKRhb deleted the contentType-fix branch May 23, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants