Skip to content

refactor: use cesdk #391

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

Merged
merged 5 commits into from
Dec 1, 2021
Merged

refactor: use cesdk #391

merged 5 commits into from
Dec 1, 2021

Conversation

grant
Copy link
Contributor

@grant grant commented Nov 18, 2021

Refactors the Node FF to use interfaces from the CESDK. No runtime change.

  • Introduces Node CESDK to Node FF
  • Use CESDK for TS interface
  • Does not use CESDK for other functionality right now

CC: @lance
Fixes: #140

Signed-off-by: Grant Timmerman <[email protected]>
@grant grant self-assigned this Nov 18, 2021
@google-cla google-cla bot added the cla: yes label Nov 18, 2021
Signed-off-by: Grant Timmerman <[email protected]>
Signed-off-by: Grant Timmerman <[email protected]>
@grant grant mentioned this pull request Nov 19, 2021
@grant grant requested a review from anniefu November 19, 2021 00:06
@matthewrobertson
Copy link
Member

I actually don't think we should take a dependency on the CESDK for this. It drags in transitive dependencies that aren't actually used at run time.

@lance
Copy link

lance commented Nov 19, 2021

It drags in transitive dependencies that aren't actually used at run time.

It does have two dependencies, true. But they are small and not unused. ajv is used for event validation and uuid is used to generate IDs if not provided. Soon, I think uuid can probably go now that there is crypto.randomUUID(). In any case, it is of course true that you know your needs best. Personally, I think the community as a whole would benefit, but that's just my opinion and likely not a motivator for you.

@lance
Copy link

lance commented Nov 19, 2021

@grant
Copy link
Contributor Author

grant commented Nov 19, 2021

I actually don't think we should take a dependency on the CESDK for this. It drags in transitive dependencies that aren't actually used at run time.

Thanks for the comment @matthewrobertson . While I understand that it's generally beneficial to reduce dependencies across our libraries, there are reasons why I believe now is a good time to add the SDK here. (this PR, including the CESDK in the FF has been a couple years in the process 😅)

Here are example specific details why we should add the SDK the the FF (and why we have done so for other FFs):

  • Without using the SDK, a developer has incompatible TS interfaces for CloudEvent data. By using the official SDK, a developer can keep using the SDK and this FF without diverging interfaces.
  • The goal of the community standard CESDKs is to fulfill this need. We've done so with 6/7 of the FFs with their transitive deps. If there's a specific dep of the CESDK, please call that out and we can go resolve that issue.
  • While this initial PR doesn't use runtime features right now, there's benefit with using the same interfaces as the SDK for all CloudEvent consumers. That's partially why we have these common CE SDK and standard.

@lance and I have made a good effort in reducing dependencies in the past. I've evaluated all deps including ajv and uuid a few months ago and we resolved any concerns there (see cloudevents/sdk-javascript#96).

In this case, I believe this PR should be reviewed as it would be beneficial to the FF authors and users.

@grant
Copy link
Contributor Author

grant commented Dec 1, 2021

Per discussion with Matt, letting Annie review as a 3rd person. I think there will be benefit for us not having to maintain CloudEvent-specific / vendor-agnostic logic within this library. We'll work with the CloudEvents community across our FFs to support the ideal devX.

@grant grant merged commit ee2c747 into master Dec 1, 2021
@grant grant deleted the grant_cloudevent_sdk branch December 1, 2021 18:54
grant added a commit that referenced this pull request Dec 1, 2021
grant added a commit that referenced this pull request Dec 2, 2021
grant added a commit that referenced this pull request Dec 2, 2021
@grant
Copy link
Contributor Author

grant commented Dec 2, 2021

Keeping record here. Matt wanted to add some comments in and wasn't able to with this PR before it was merged. We talked about this PR last Friday and some more today where we have a review in the quick roll-back roll-forward PR. Proceeding with the roll-forward as discussed.

grant added a commit that referenced this pull request Dec 2, 2021
* Revert "Revert "refactor: use cesdk (#391)" (#399)"

This reverts commit ad75658.

* ci: run doc generator

Signed-off-by: Grant Timmerman <[email protected]>

* ci: gen docs

Signed-off-by: Grant Timmerman <[email protected]>

* refactor: minor import refactoring

Signed-off-by: Grant Timmerman <[email protected]>

* ci: fix docs

Signed-off-by: Grant Timmerman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use CloudEvents SDK
4 participants