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

Breaking changes in DisplayManager in 1.5 #12678

Closed
ns8482e opened this issue Oct 21, 2022 · 16 comments · Fixed by #12689
Closed

Breaking changes in DisplayManager in 1.5 #12678

ns8482e opened this issue Oct 21, 2022 · 16 comments · Fixed by #12689

Comments

@ns8482e
Copy link
Contributor

ns8482e commented Oct 21, 2022

Describe the bug

I am using BuildEditor of DisplayManager to create a shape, It's working well, but when upgraded to 1.5 preview feed started getting following error.

image

@hishamco
Copy link
Member

Could we add it to 1.5 milestone and handle it before the release?

@MikeAlhayek
Copy link
Member

@hishamco nothing to be done here. Maybe just add documentation. PR #11775 added string htmlPrefix = "" to the IDisplayManager that seems to break any project that depends on < 1.5 but also used 1.5 in other project.

@hishamco
Copy link
Member

@ns8482e could you please add docs for this?

2 similar comments
@hishamco

This comment was marked as duplicate.

@hishamco

This comment was marked as duplicate.

@hishamco hishamco added this to the 1.x milestone Oct 21, 2022
@ns8482e
Copy link
Contributor Author

ns8482e commented Oct 21, 2022

@hishamco it should be backward compatible, it breaking the module can you change the milestone to 1.5? I’ll create PR tonight

@hishamco
Copy link
Member

@MikeAlhayek is not backward compatible?

@ns8482e
Copy link
Contributor Author

ns8482e commented Oct 21, 2022

@hishamco No html prefix changes are not backward compatible, module written on 1.4 can’t find the method as param of API function has changed and now it’s complaining about method not found exception

@hishamco
Copy link
Member

Hope if you can fix this before 1.5 is release, as I expect it will be ASAP

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Oct 21, 2022

@ns8482e your module depends on v1.3 so you can add dependency to your module for OC <=1.4. then create another package that depends on >=1.5. There is is really no to do in OC for this.

The issue is that your project is marked as compatible with 1.3+ when it's not. your current release need to depend on OC 1.2> and <= 1.4. This way your customer won't be able to update the package if they are using 1.5

@ns8482e
Copy link
Contributor Author

ns8482e commented Oct 21, 2022

@MikeAlhayek It's not just about my modules, It about how how OC Framework handles breaking changes gracefully in minor version upgrades.

@ns8482e
Copy link
Contributor Author

ns8482e commented Oct 22, 2022

project is marked as compatible with 1.3+ when it's not

:) Can't predict that OC 1.5 will introduce breaking changes when released module 1.3 released

@ns8482e
Copy link
Contributor Author

ns8482e commented Oct 22, 2022

@hishamco I guess this can't be fixed as @MikeAlhayek suggests -requires the module to be recompiled

@hishamco
Copy link
Member

Niraj please do the right fix, then you can request @jtkech

@ns8482e
Copy link
Contributor Author

ns8482e commented Oct 22, 2022

It's compiler - It doesn't allow me to add overload method as it thinks ambiguous.

@jtkech original interface was as below

Task<IShape> BuildEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = "");
Task<IShape> UpdateEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = "");

That changed to following in 1.5-preview

 Task<IShape> BuildEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = "", string htmlPrefix = "");
 Task<IShape> UpdateEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = "", string htmlPrefix = "");

So all calls from module complied and released before 1.4 expects a method that has four params that results in method not found in when upgraded OC to 1.5-preview.

Here in new method newly added default parameter string htmlPrefix = "" is not backward compatible and adding overload like Task<IShape> BuildEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = ""); results in ambiguous compile error.

@ns8482e
Copy link
Contributor Author

ns8482e commented Oct 22, 2022

@hishamco @jtkech figured way fix it by keeping htmlPrefix without default value PR #12689

@ns8482e ns8482e modified the milestones: 1.x, 1.5 Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants