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

Add migration guide for v2 #1327

Merged
merged 13 commits into from
Nov 13, 2024
Merged

Add migration guide for v2 #1327

merged 13 commits into from
Nov 13, 2024

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Nov 11, 2024

This adds a guide to the main README for migrating from Connect v1 to Connect v2.

@smaye81 smaye81 force-pushed the sayers/migration_guide branch from 7c85665 to f0a772e Compare November 11, 2024 16:43
README.md Outdated Show resolved Hide resolved
@smaye81 smaye81 marked this pull request as ready for review November 11, 2024 16:45
@smaye81 smaye81 changed the title Migration guide for v2 Add migration guide for v2 Nov 11, 2024
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

WDYT about moving this to a separate markdown file? A year down the road, we probably don't want to have the guide take real estate on the main README, but it would be good to have stable links. A separate file would solve this.

Other changes users are likely to run into:

  • When creating a message to pass in a request field, the syntax changes quite a bit. It's common outside of simple examples, so we have to show an example.
  • If you use a BSR dependency for custom options, generated code will reference it. This means you have to generate with include_imports: true. A good example is any message that uses protovalidate.
  • PlainMessage - we can repeat the information from the Protobuf-ES migration guide.
  • WKT methods - for example, converting a google.protobuf.Timestamp to an ECMAScript Date object is no longer possible via a method. We need to call out the helpers exported from @bufbuild/protobuf/wkt, because they are not easily discoverable. Similar for google.protobuf.Any.

Also notable:

  • In general, request and responses do not need any changes when initializers / dot notation is used, but google.protobuf.Struct is now generated as a more convenient JsonObject, and proto2 fields with default values are no longer generated as optional properties.
  • SSR is generally much more simple because it doesn't require any extra steps (with proto3). This is difficult to explain in all details with a diff, but we really need to explain the general changes.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@timostamm
Copy link
Member

@paul-sachs
Copy link
Contributor

One thing I noticed with my migration in the BSR is that the usage of helper types like PartialMessage and PlainMessage have been changed. I think we should have some guidance on how to replace those as well. Something like:

`PlainMessage` is no longer required since all messages are plain objects. You may need to remove the $typeName field if using one message type in place of another.

`PartialMessage` is more or less replaced with `MessageInitShape` but requires the schema type input.

Not exactly sure where to put those but feels like we should mention them.

@smaye81
Copy link
Member Author

smaye81 commented Nov 12, 2024

OK, I think I incorporated everything into this round 2 of changes. The only thing I didn't add is the bit about SSR since that isn't really a migration change. @timostamm let me know where you think it fits best (or if it makes more sense to just link to connect docs for SSR)

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Nice! Left a couple of suggestions below.

the bit about SSR

I think we do need examples for how SSR changes, and guidance for replacing usage of PlainMessage and PartialMessage. But we can do that in a separate PR.

MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Show resolved Hide resolved
@smaye81
Copy link
Member Author

smaye81 commented Nov 13, 2024

@timostamm ready for round 3.

@smaye81 smaye81 requested a review from timostamm November 13, 2024 17:04
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

LGTM!

Took a note to look into better guidance for PlainMessage and PartialMessage and to add examples for SSR.

Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
@smaye81 smaye81 force-pushed the sayers/migration_guide branch from 543394a to d2acd07 Compare November 13, 2024 17:56
@smaye81 smaye81 merged commit bc4a5ac into main Nov 13, 2024
40 of 44 checks passed
@smaye81 smaye81 deleted the sayers/migration_guide branch November 13, 2024 18:01
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.

3 participants