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

fix: HydratedBloc storage doesn't change without closing previous storage #3887

Open
mahdi739 opened this issue Jun 23, 2023 · 8 comments
Open
Assignees
Labels
bug Something isn't working pkg:hydrated_bloc This issue is related to the hydrated_bloc package

Comments

@mahdi739
Copy link

mahdi739 commented Jun 23, 2023

Description
When I try to change the HydratedBloc.storage variable, It won't change, unless I close the previous storage.

Steps To Reproduce

  1. Create a hydrated bloc (cubit)
  2. Build and set a HydratedStorage with a path
  3. Change the state of bloc
  4. Build and set a different HydratedStorage with a new path
  5. Renew the Bloc(cubit) object;
  6. It supposed to have the initial state but it load the state from previous storage which means the HydratedStorage is not changed.

Simple code to reproduce it:

import 'dart:io';
import 'package:hydrated_bloc/hydrated_bloc.dart';

class CounterCubit extends HydratedCubit<int> {
  CounterCubit() : super(0);

  void increment() => emit(state + 1);

  @override
  int fromJson(Map<String, dynamic> json) => json['value'] as int;

  @override
  Map<String, int> toJson(int state) => {'value': state};
}

Future<void> main(List<String> arguments) async {
  HydratedBloc.storage = await HydratedStorage.build(storageDirectory: Directory("source_1")..createSync());
  var counterCubit = CounterCubit();
  counterCubit.increment();
  print(counterCubit.state); // 1 ✔
  HydratedBloc.storage = await HydratedStorage.build(storageDirectory: Directory("source_2")..createSync());
  counterCubit = CounterCubit();
  counterCubit.increment();
  print(counterCubit.state); // 2 ❌
}

We have to close the previous storage, however If we don't, I think the method should do it automatically or throw an exception.
When nothing changes and no exceptions are thrown, debugging can be difficult.

@mahdi739 mahdi739 added the bug Something isn't working label Jun 23, 2023
@felangel felangel added the pkg:hydrated_bloc This issue is related to the hydrated_bloc package label Nov 7, 2023
@codesculpture
Copy link
Contributor

I guess, it safe to throw error if we try to build storage when already a storage lives (i.e not closed) ?. @felangel

@felangel
Copy link
Owner

felangel commented Nov 27, 2023

I guess, it safe to throw error if we try to build storage when already a storage lives (i.e not closed) ?. @felangel

That seems reasonable. The more user-friendly option in my opinion would be to automatically close the previous storage and register the new storage since that's likely what users would want in this case. Wdyt?

@codesculpture
Copy link
Contributor

Well thats what shld be done, am wondering does the user are aware that if they build new storage and existing one would deleted without doing "clear". Since as of now, user may know the only way to delete the storage instance is by doing "clear". Well i agree to clear automatically, but wondering that doesnt bother existing users who are using already. Does this be considered be breaking change.

@felangel
Copy link
Owner

Well thats what shld be done, am wondering does the user are aware that if they build new storage and existing one would deleted without doing "clear". Since as of now, user may know the only way to delete the storage instance is by doing "clear". Well i agree to clear automatically, but wondering that doesnt bother existing users who are using already. Does this be considered be breaking change.

Technically it would be but this seems more like a bug since I can't imagine many people would expect/rely on the current behavior. I would personally treat this as a bug fix since the current behavior is incorrect.

@codesculpture
Copy link
Contributor

Well agreed to clear the existing one, will write a PR asap and will let u know. Thanks for ur time felix

@felangel
Copy link
Owner

Well agreed to clear the existing one, will write a PR asap and will let u know. Thanks for ur time felix

Thanks for your time and help as always! 🙏

@codesculpture
Copy link
Contributor

codesculpture commented Feb 17, 2024

Its been a while we talked about this, the fix am trying to land
That would introduce this, at a single time there must only one storage can live with the same path.
If anyone need to create a storage in the same the path, they must close the existing one but if they
fail to, there is two storage points to same path, which could wait forever due to another storage is using already would throw error.

But as of our current impl, if user already building the multiple storage at same path its not breaking due to the fact we are just handing the same storage instance even if they try to create multiple times.

So after this fix, if one might using in this way, the app could break. But this is totally wrong behavior and the right thing is to throw if they using same storages at same path at a same time. What do u think @felangel :/ .

Btw, its been so long we talked about this 😂

@xoliq0v
Copy link

xoliq0v commented Nov 17, 2024

Hi! 👋

Thanks for pointing this out. I see how changing the HydratedBloc.storage variable without closing the previous storage can lead to unexpected behavior, as the state doesn’t reset when switching to a new HydratedStorage.

Suggested Workaround

As you mentioned, closing the previous storage before creating a new one resolves this issue. You could add an explicit call to close the current storage before assigning a new one:

await HydratedBloc.storage?.close();
HydratedBloc.storage = await HydratedStorage.build(storageDirectory: Directory("source_2")..createSync());

Proposed Enhancement

It would be useful if HydratedBloc.storage either:
1. Automatically closed the current storage when assigning a new one, or
2. Threw an exception if there’s an attempt to assign a new storage without closing the previous one.

Implementing one of these approaches would help prevent silent issues and make debugging easier by enforcing best practices around storage management.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:hydrated_bloc This issue is related to the hydrated_bloc package
Projects
None yet
Development

No branches or pull requests

4 participants