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

Use Direct Object Creation Pattern, Do Not Use Builder Pattern #65

Closed
grant opened this issue Apr 27, 2020 · 6 comments · Fixed by #172 · May be fixed by veekungx/sdk-javascript#2
Closed

Use Direct Object Creation Pattern, Do Not Use Builder Pattern #65

grant opened this issue Apr 27, 2020 · 6 comments · Fixed by #172 · May be fixed by veekungx/sdk-javascript#2
Assignees
Labels
type/enhancement New feature or request

Comments

@grant
Copy link
Member

grant commented Apr 27, 2020

The JavaScript SDK uses a Java-like builder pattern for creating events.

This is not ideal as it is not very JavaScript-like, and is unnecessarily wordy. As we are in JavaScript, we can use direct JSON objects for creating the CloudEvent payload.

Here is a sample taken from the README:

Actual

let myevent = event()
  .source('/source')
  .type('type')
  .dataContentType('text/plain')
  .dataschema('http://d.schema.com/my.json')
  .subject('cha.json')
  .data('my-data')
  .addExtension("my-ext", "0x600");

Expected

import {CloudEvent} from 'cloudevents/v1';
let myevent = new CloudEvent({
  source: '/source',
  type: 'type',
  dataContentType: 'text/plain',
  dataschema: 'http://d.schema.com/my.json',
  subject: 'cha.json',
  data: 'my-data',
  addExtension: ["my-ext", "0x600"]
});
@fabiojose
Copy link
Contributor

LGTM

@lance lance mentioned this issue Apr 28, 2020
@lance
Copy link
Member

lance commented Apr 28, 2020

I agree that this would be an improvement in the API. It makes me think about versioning. This is a breakable change to the API. So, presumably it would need a major version number bump. Does this become problematic reconciling the module version with the spec version? What is the thinking here?

@grant
Copy link
Member Author

grant commented Apr 28, 2020

Good questions.
The npm version (CloudEvent client) and specversion may differ. As such, the npm module version may be higher than the CE spec (they should be unrelated). We don't really expect the Node client to only be changed when there's a major release in the CE spec of course.

I've updated the exact sample to be more consistent with Go and normal versioning. When v2 comes out, I would assume you import or require v2.

@fabiojose
Copy link
Contributor

I agree that this would be an improvement in the API. It makes me think about versioning. This is a breakable change to the API. So, presumably it would need a major version number bump. Does this become problematic reconciling the module version with the spec version? What is the thinking here?

No problems @lance, we may have versions higher than spec. Now we are are a kit and we have our own lifecycle that supports some spec versions and features.

@lance lance added the type/enhancement New feature or request label May 9, 2020
@lance
Copy link
Member

lance commented May 12, 2020

@grant @fabiojose should we consider trying to get this in for a 2.0.0 release? I think it's a nice change, and since we are making big API changes with the upcoming release, this might be the place for it. My only concern is that this is probably a pretty big change, touching lots of the underlying code. It might be a big PR that takes some time to hash through. WDYT?

@grant
Copy link
Member Author

grant commented May 12, 2020

@lance Glad you like the proposed change.

So long as there are tests, any number of PRs to make the change are fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement New feature or request
Projects
None yet
3 participants