[VRouter] simplifying dynamic linking scenario

flutter / uxr

UXR work for Flutter

BSD 3-Clause "New" or "Revised" License

229 stars 28 forks source link

[VRouter] simplifying dynamic linking scenario #47

Closed lulupointu closed 2 years ago

lulupointu commented 3 years ago

This is a small commit to simplify the dynamic linking scenario base on https://github.com/flutter/uxr/issues/40#issuecomment-821306338.

I know you don't want to take Dynamic Linking into account for the final review but since this repo is also meant for devs to come and compare the different scenario I think this will be useful. In any case, changes are small so it should not take too long to review.

johnpryan commented 3 years ago

@jackkim9 is it okay to change the snippets after the walkthrough study?

lulupointu commented 3 years ago

Note that these change concern more the Flutter logic than vrouter per say. The routes are the same, I mainly remove the onCreate function which was causing trouble during the study (which makes sense since I should not have used it in the first place)

xuanswe commented 3 years ago

I have some questions regarding how should we write the code snippets for packages.

What's the purposes of the snippets in details?
With that purposes,
- Should we keep changes minimal from the original code?
- Or should we make the most optimized code for each package?
How we keep the study result accurate but still allow improvements of packages?

IMO, if we change the code, which was used in the study, the result is not correct anymore. So I don't think we should allow that. But packages are usually improved overtime, so we could give room to authors to telling that their packages are now improved. In this case, I think we need to create another file for the latest version, so we don't need to touch the old one, which connected to the study result.

InMatrix commented 3 years ago

@nguyenxndaidev These are good questions. When we publish study results, we should identify a specific commit for each snippet used in the study. The findings will have a shelf life, since those packages will evolve. IMO, package authors can continue updating their snippets in this repo, but they should provide a summary of changes and explain whether any of the changes are intended to address findings from the study. No changes should affect the demo's behavior though.

InMatrix commented 3 years ago

@lulupointu Would you still be interested in merging this PR? The folder structured has changed, so if you'd like to merge this PR, please update it to match the structure on master. Thanks.

lulupointu commented 3 years ago

Sure, I would also need to update the other scenarios because there has been breaking changes since I posted them. Should I make a separate PR or update this one with everything?

InMatrix commented 3 years ago

A separate PR might be cleaner. Thanks.

InMatrix commented 2 years ago

@lulupointu I'm closing this PR for now. Feel free to open a new one when you're ready for updating your snippets. Thanks!