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

Minor improvements with small breaking changes #2087

Merged
merged 31 commits into from Jan 5, 2022

Conversation

ahmednfwela
Copy link
Contributor

This contains some minor improvements and a few breaking changes

  1. Added splash screen example to Nav2 api
  2. Added PageBindings that pass GetPageRoute as a parameter to the dependencies method (see added PageBindings that references GetPageRoute in [dependencies] method #1722)
  3. Added optional pickPagesForRootNavigator to GetDelegate , will be explained in the documentation WIP nav2 documentation #2032
  4. [BREAKING] Fixed ValueBuilder throwing due to incorrect nullability handling
  5. [BREAKING] Fixed inconsistent responsive calculation behavior by introducing more explicit APIs
    • isPhoneOrLess
    • isPhoneOrWider
    • isPhone falls back to isPhoneOrLess (old behavior, non breaking)
    • isSmallTabletOrLess
    • isSmallTabletOrWider
    • isSmallTablet falls back to isSmallTabletOrLess (BREAKING)
    • isLargeTabletOrLess
    • isLargeTabletOrWider
    • isLargeTablet falls back to isLargeTabletOrLess (BREAKING)
    • isDesktopOrLess
    • isDesktopOrWider
    • isDesktop falls back to isDesktopOrLess (BREAKING)
    • Also the comparison now works based on the width not the shortest side, which is the consistent way to build the UI
      • design is built based on the known width breakpoints
      • height is assumed small and scrollable
    • Also changed the tests to the new behavior
  6. [BREAKING] changed GetDelegate api signature to return Future<void> instead of Future<T> , if the user wants to get a value from a route, they should either use services or Get.dialogs, the old API was returning a value from a completer, and it wasn't consistent across browser refreshes, so it's for the best they get removed altogether

Pre-launch Checklist

  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or @jonataslaw said the PR is test-exempt.
  • All existing and new tests are passing.

@ahmednfwela ahmednfwela marked this pull request as ready for review December 28, 2021 03:12
@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Dec 28, 2021

@jonataslaw please review this, I would like these to get merged before the docs PR

@jonataslaw
Copy link
Owner

Wow, thanks for that.
Many cool changes.
I will review this and merge soon.
As there are breaking changes, it might be better to add this to GetX 5.
I don't know if we are going to create a branch for it, or already use the master branch directly.
I've already refactored GetBuilder, and added a new feature that makes SM with Scopo (just like Provider, where you can link a context with a controller without having to wrap it in a new GetBuilder, and decrease the amount of StatefulWidgets per tree, since GetBuilder was a StatefulWidget), and at the same time we will allow this controller to be accessible outside the tree, different from the provider, allowing testing dependency injection with unit tests.
I'm testing something new with Obx right now, I really wanted to join the streams api to the notifiers api, but this is not such a simple task

@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Dec 28, 2021

I just saw the Scope additions, and they are really nice, also what do you think about adding an Obx.context/GetX.context named constructors that pass the BuildContext, e.g.

Obx.context((context) {
//return widget
})

@jonataslaw
Copy link
Owner

I just saw the Scope additions, and they are really nice, also what do you think about adding an Obx.context/GetX.context named constructors that pass the BuildContext, e.g.

Obx.context((context) {
//return widget
})

This is something I would like to discuss.
I think we need to demystify the use of context a bit, there are a ton of things like themes, mediaquery, that need it, because we can access this data directly through ui.window, however, the context does the automatic update in case of change.
I know a lot of people love the framework because they don't need to use boilerplate code adding context to things that don't depend on updates (like navigation, where the context is just used to go up the tree in search of NavigatorState).
My opinion remains the same as for the second case, we don't need to use InheritedWidget for this.
In all situations where getElementForInheritedWidgetOfExactType is used (where the context is just used to climb the tree, but not for the purpose of listening for changes), I really think we can use our instance to distribute these dependencies.
However wherever it is used: dependOnInheritedElement, context is required.
Using Obx, GetX and GetBuilder without context often leads to having access to a context above it to get these changes.
That's why I'm thinking about including the context in Obx, GetX, and GetBuilder.
Having a builder for him is also a good idea, which will bring less breakage. I'm still thinking about what we should do in version 5. There are projects that have more than 600 Obx (my company's, for example), and making such a sudden change might cause more problems than a solution.
What do we have to decide then:
1- We added context to the 3 widgets and created a migration tool in our CLI that replaces: Obx(()=> with Obx((_)=>
2- We created a new widget, which has context, and we recommend it.
3- We use a new builder.

The point is that not using context within builders is a Flutter anti-pattern, and I wanted to get rid of anti-patterns in version 5. However I know a lot of the community likes the convenience of using a context-free wrapper, and often they want to update a component that doesn't need to access MediaQuery, Theme, or anything the context is required.
I cannot decide this alone.

@jonataslaw
Copy link
Owner

Using context, MixinBuilder totally loses its usefulness too and can be removed, with GetBuilder using Scope we have a single listener being delivered by the whole tree instead of several StatefulWidgets subscribing to a listener. This is a significant performance improvement and the best part is being able to subscribe listeners inside any widget that has context. We can listen for an update() inside an Obx, if it had context (the problem is that it doesn't pass this context by parameter).
Another thing I want to optimize is to remove duplicate apis.
Currently we have SimpleBuilder and Obx that do the same thing, one with .obs and the other with .reactive. One uses streams, and another uses notifiers (VoidCallback Lists), and I'm thinking about maybe moving to a single api to reduce the codebase.
I like flexibility, but since we have a lot of widgets to do the same thing, it becomes a problem.

Maybe we'll also just use navigator 2 (this will require more care in it), and there are things you just can't do with it, like route response, which will break compatibility. We need migration guides from the old Get.back(result: 'bla') to navigator 2.

There are so many things to discuss. This is the first big change. We have had virtually no break versions since version 1, the breaks were so subtle. In this version I think things are going to be a little heavier.

@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Dec 29, 2021

@jonataslaw there is no need for breaking changes at all, look at this branch I made:
https://github.com/Bdaya-Dev/getx/tree/state_management_playground
commit:
Bdaya-Dev@496348b

you can introduce new api while maintaining backward compatibility with old api

@ahmednfwela
Copy link
Contributor Author

as for the navigation API, I suggest we leave it as is.
some people might want to use the old API, others might want to use the new one.
we will just have to clarify the usage in the docs

@shubhamsinha2009
Copy link

@jonataslaw Great , i think if you can add get cli for replacing all previous obx with obx.context , you should definitely do it . It will be major breaking change as well as anti- Flutter problem will be solved

@jonataslaw
Copy link
Owner

Captura de Tela 2021-12-29 às 23 20 14

Bindings api will change

BindingsBuilder will be replaced too

@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Dec 30, 2021

I like the bindings changes, can you make it return an iterable instead of list ?

@jonataslaw
Copy link
Owner

I like the bindings changes, can you make it return an iterable instead of list ?

We can. Is there any specific reason for this?

@jonataslaw
Copy link
Owner

I think the changes I made to the Bindings ended up conflicting.

@ahmednfwela
Copy link
Contributor Author

We can. Is there any specific reason for this?

well, since users are required to override this method, it's good to always require the least restrictive type (a.k.a. Iterable), also you can use generators with it (sync*)

@ahmednfwela
Copy link
Contributor Author

I think the changes I made to the Bindings ended up conflicting.

I will solve it

@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Dec 31, 2021

your changes are going to break the public API for the middleware though @jonataslaw

@jonataslaw
Copy link
Owner

Our next update will be a major change, so we can change the current api without fear

@jonataslaw
Copy link
Owner

jonataslaw commented Jan 3, 2022

Would it be possible to leave isDesktop/isTablet/isPhone as it used to be?
I think the additions you made by adding OrWider and OrLess are enough, and in this PR isDesktop becomes just a getter for another function, so we could have both apis

@ahmednfwela
Copy link
Contributor Author

but the old API would be inconsistent, isPhone was isPhoneOrLess while isDesktop was isDesktopOrWider

I think it's better to make a breaking change now which will be better in the long run

@jonataslaw
Copy link
Owner

Actually on this code on a phone with the horizontal orientation, and isPhone == false.
Tried on an upright tablet, and isPhone == true.

@ahmednfwela
Copy link
Contributor Author

well, it makes sense, since the correct way to check is using checkpoints, not binary if

@jonataslaw
Copy link
Owner

I think we should keep both apis.
Or create something with "strict" or another nameclature to define the height and length in vertical mode only, and another for both.

As for the dependency manager, it sounds good to me,
Changes in routes too
Just that I think needs some change.

@jonataslaw
Copy link
Owner

Sorry, I have timelines to comply, I will accept the PR, but make the required changes

@jonataslaw jonataslaw merged commit 70a34a6 into jonataslaw:master Jan 5, 2022
@ahmednfwela
Copy link
Contributor Author

can you please publish it as 5.0.0-alpha1 ?
we need to test it before it's in production

@jonataslaw
Copy link
Owner

Just to let you know.
Binding will need to revert to calling a List instead of an Iterable.
Binds will need to call binds.reversed.fold, and iterable does not have these properties.

@ahmednfwela
Copy link
Contributor Author

@jonataslaw why not leave the public API as Iterable and convert it to list internally?

@jonataslaw
Copy link
Owner

This also breaks GetPageRoute. It's not impossible to do, but we have a lot of changes to make.

@ahmednfwela
Copy link
Contributor Author

can I make a PR to rewrite the public API?

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.

None yet

4 participants