refactor: simplify http emitter #310
Conversation
Signed-off-by: Grant Timmerman <[email protected]>
There was a problem hiding this comment.
I don't think this is a good change to make. It changes the user facing API by moving exports into http/emitter, which will require changes to client code - therefore resulting in at least a minor version bump. A minor version bump is not in itself bad, but there does not appear to be any benefit to this. Nothing has changed other than binary_emitter.ts and structured_emitter.ts have been moved into one file, and you've inlined the emit() function.
I don't see any good reason for this to land.
|
Hi, I understand that there are changes to the filename that affect the API. I created #314 to describe a bit of what I'm talking about. I also added more detail to the PR description. This change is required for eventually creating an easier to use emit function. |
This change is not reducing duplicated code.
You can already simply emit() a CloudEvent. |
|
There is duplicated code in imports. The diff count is negative. What would you like me to change in this PR? |
Nothing. I don't want it to land because it's a breaking change with no obvious benefit. If it's in preparation for an actual beneficial change, then submit that actual beneficial change with this included. I'm not opposed to breaking changes in the abstract, but in this case there really is no benefit at all to the end user and so I don't think it should land until there is one. |
|
I'm purposefully creating small PRs that don't break tests. I can't create a larger PR for this. They would depend on this. (Believe me, I've tried.) Do you disagree that removing duplicate code and duplicate exports is not a benefit? |
I'm curious why you can't create a large PR that takes care of all the refactoring you feel is necessary? I know you mentioned not wanting to break tests, but just fix the tests then in that same PR. When we moved to typescript, that was a fairly large PR, and the tests broke and were re-written to use the new APIs As for this PR, i get the idea of wanting to remove duplicate code, but this is making a breaking change for the end user, with no added benefit for the end user. It might simplify our life as a maintainer, but the end user is now having to change thier code and they don't know why. yes they had to change there code when updating to 3.x from 2.x, but 1, that was a major release, and 2, they could see the benefit and understood why they needed to change that code. |
|
Hi folks, Fundamentally we're replacing 2 exports with 1 export for the same HTTP emitter. We've already done this in other files in different PRs. Besides the not exporting twice, the PR is a refactor. |
it is not as simple as that statement. Yes, you are combining 2 exports with 1, but you are also introducing a breaking change for the user, with not really any benefit to them as I mentioned in my previous comment.
I also wanted to address this statement. This is not a yes or no answer. it depends on the circumstances. I would say generally yes, less code is good, but only when it doesn't break things for an end user. And in this case, as said before, it is breaking things, for a small cleanup |
|
I'm not sure what you want me to do. |
And the PRs that have had breaking changes, have added value to the end user, like the big Typescript re-write. I think to land this, we would have to wait until a 4.0 release. |
|
4.0 is fine. How can I help land this in 4.0? |
I recommend closing this PR and making these changes as a part of another PR that adds tangible value to the user. |
|
Will just close. I really really think that small PRs are okay. Eventually whoever tackles #314 can just take this PR and make it a bigger PR. When I tried to do that, I ran into issues modifying too many tests, so it may take time. |
Proposed Changes
emit()a CloudEvent.