[shared_preferences] Add shared preferences devtools

[adsonpleal]

This PR adds the shared_preferences_tools package. This package user the devtools_extension tooling to create a tool for shared preferences. The idea of this PR came from @kenzieschmoll on this issue. Initially I've published this tool as a separate package, but this PR aims to bring the functionality to the main shared_preferences package. Once this PR gets merged I'll archive the shared_preferences_tools package.

https://github.com/flutter/packages/assets/11666470/fcf71145-c330-4397-b62e-c0c4c8bc9f01

Pre-launch Checklist

• [X] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [X] I read the Tree Hygiene wiki page, which explains my responsibilities.
• [X] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• [X] I signed the CLA.
• [X] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• [X] I linked to at least one issue that this PR fixes in the description above.
• [X] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• [X] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• [X] I updated/added relevant documentation (doc comments with ///).
• [X] I added new tests to check the change I am making, or this PR is test-exempt.
• [X] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[adsonpleal]

@kenzieschmoll Done, all the requested changes were applied. One thing that I'd like to do, though, is some integrations tests. I was exploring the devtools github repo and saw that you already have a devtools_test package, but it doesn't have the fixture tooling used in the devtools/devtools package. The best scenario would be to be able to create fixture tests like this one.

Do you have plans of extracting this tooling to the devtools_test package? Or is there some docs on how to do fixture tests for devtools_extensions packages?

[kenzieschmoll]

How long does that build take?

@stuartmorgan depending on the size of the app, maybe a minute or two? The build command builds the extensions flutter web app in release mode, and then copies the assets to the extension/devtools/build folder.

[stuartmorgan-g]

Adding a minute or two to the publish step is fine. We'll need to add repo tooling for packages to be able to specify a pre-publish hook, probably via a tools/some_known_name.dart script that does whatever needs to be done.

[adsonpleal]

@kenzieschmoll and @stuartmorgan also the tests need to run with the --platform chrome flag since I'm doing some UI tests and the devtools_extensions won't run without this flag.

flutter test --platform chrome

[kenzieschmoll]

@kenzieschmoll Done, all the requested changes were applied. One thing that I'd like to do, though, is some integrations tests. I was exploring the devtools github repo and saw that you already have a devtools_test package, but it doesn't have the fixture tooling used in the devtools/devtools package. The best scenario would be to be able to create fixture tests like this one.

Do you have plans of extracting this tooling to the devtools_test package? Or is there some docs on how to do fixture tests for devtools_extensions packages?

We are not planning on publishing the devtools_test package, but devtools_shared does have a library devtools_test_utils.dart that should what you are looking for.

You can use the integration test of the simulated environment in package:devtools_extensions as a guide: https://github.com/flutter/devtools/tree/master/packages/devtools_extensions/integration_test.

[kenzieschmoll]

@stuartmorgan WRT to testing, is there a way to add an integration test hook to the CI? For example, the flutter/devtools CI has this code to run the flutter web integration tests for the devtools_extensions package: https://github.com/flutter/devtools/blob/master/tool/ci/bots.sh#L91-L94

[stuartmorgan-g]

@stuartmorgan WRT to testing, is there a way to add an integration test hook to the CI? For example, the flutter/devtools CI has this code to run the flutter web integration tests for the devtools_extensions package: https://github.com/flutter/devtools/blob/master/tool/ci/bots.sh#L91-L94

It's possible to run arbitrary bespoke tests with tool/run_tests.dart, but that's generally an approach of last resort. What specifically do the tests here need that isn't covered by standard integration test invocation?

[kenzieschmoll]

@stuartmorgan WRT to testing, is there a way to add an integration test hook to the CI? For example, the flutter/devtools CI has this code to run the flutter web integration tests for the devtools_extensions package: https://github.com/flutter/devtools/blob/master/tool/ci/bots.sh#L91-L94

It's possible to run arbitrary bespoke tests with tool/run_tests.dart, but that's generally an approach of last resort. What specifically do the tests here need that isn't covered by standard integration test invocation?

What is the "standard" integration test invocation? The example I linked from package:devtools_extensions runs this flutter drive command under the hood: https://github.com/flutter/devtools/blob/master/packages/devtools_shared/lib/src/test/integration_test_runner.dart/#L39-L53

This extra layer provides us with things like enhanced logging, retry, and sharding. We are using this shared utility in package:devtools_extensions mostly because this is how we have to do it for devtools_app, since for those tests, we have to have additional logic that spins up a test application to connect DevTools to while it is running the integration test.

[stuartmorgan-g]

https://github.com/flutter/packages/blob/main/script/tool/lib/src/drive_examples_command.dart is this repo's CI driver for integration tests. I just realized this package isn't a library with an example/ though, so we'd need to make it understand top-level app packages, or use custom_test.

[adsonpleal]

@kenzieschmoll I've implemented some changes to use a InheritedModel instead of a InheritedNotifier to avoid unnecessary rebuilds. Could you take a look at the changes and let me know what you think?

[stuartmorgan-g]

The last thing I think we need to do before landing this

Before the PR lands, either the shared_preferences changes need to be reverted, or the repo tooling to do custom pre-publish hooks need to be written. Right now this PR is changing shared_preferences to say it's adding something that is not actually being added.

[adsonpleal]

ing I think we need to do before landing this is to disable th

The last thing I think we need to do before landing this

Before the PR lands, either the shared_preferences changes need to be reverted, or the repo tooling to do custom pre-publish hooks need to be written. Right now this PR is changing shared_preferences to say it's adding something that is not actually being added.

Got it, so I'll revert the changes there and add the check that @kenzieschmoll mentioned.

There are also some checks failing here.

  The following unexpected non-local dependencies were found:
    devtools_app_shared
    devtools_extensions
    mockito

I'm assuming that for mockito I just need to pin the version, since we have it here.

But for devtools_app_shared and devtools_extensions I'd need to add them to the config. Should I add them to allowed_unpinned_deps or to allowed_pinned_deps?

It is also complaining about codeowners, I actually don't know why, I thought this would be enough to set the ownership.

[adsonpleal]

@kenzieschmoll this is how the message looks like:

[kenzieschmoll]

There are also some checks failing here.

  The following unexpected non-local dependencies were found:
    devtools_app_shared
    devtools_extensions
    mockito

I'm assuming that for mockito I just need to pin the version, since we have it here.

But for devtools_app_shared and devtools_extensions I'd need to add them to the config. Should I add them to allowed_unpinned_deps or to allowed_pinned_deps?

It is also complaining about codeowners, I actually don't know why, I thought this would be enough to set the ownership.

@stuartmorgan can you advise?

[stuartmorgan-g]

But for devtools_app_shared and devtools_extensions I'd need to add them to the config. Should I add them to allowed_unpinned_deps or to allowed_pinned_deps?

Unpinned is fine. (We may actually remove this distinction in the future now that we don't check for deprecation warnings in CI.)

It is also complaining about codeowners, I actually don't know why, I thought this would be enough to set the ownership.

Every individual package other than an app-facing package is expected to have a specific owner; the entries at the top are the coordinating owners of the overall plugin, but not expected to own every subpackage.

[kenzieschmoll]

This looks good! The last thing I think we need to do before landing this is to disable the extension for web targets (temporarily). It looks like we have a path forward to support the async evals on web, but this will only be available for users who are on a version of Dart that is after the changes that @sigmundch is working on. Since we don't have that dart SDK version right now to gate on, I recommend that we disable for web targets and come back later to fix the version gate with a patch release. You can check serviceManager.connectedApp.isDartWebApp from your DevTools extension to short circuit with an error message.

Or if you aren't in a hurry to land this, we can also wait until we have the correct Dart SDK version to gate on.

@sigmundch landed this fix: https://github.com/dart-lang/sdk/commit/9dad32ce412a1d6085e783d3d55d707f8f157ef9. @adsonpleal can you checkout Flutter at the tip of the main branch and see if you are still seeing errors from the extension when connected to a web app and performing async evals? If you are no longer seeing the issue, we can remove the web disabled check and replace it with a check that gates on the Dart SDK version that includes Siggi's change instead.

The best way to get the Dart version is by checking: serviceManager.connectedApp.flutterVersionNow.dartSdkVersion

[adsonpleal]

@kenzieschmoll the fix should be available since 3.5.0-192.0.dev, I'm on main channel with the SDK version 3.5.0-227.0.dev, but the error is still there:

[sigmundch]

Just adding a short comment since I'm unfortunately out sick for a few days.

Sorry to hear you still see the error. I am not able to investigate at the moment, but maybe in your local setup you may be able to directly get some additional hints that will help us figure out what's wrong.

It appears the error message comes from https://github.com/flutter/flutter/blob/b5697a0c6dec3218d324d0c996143ecc7decede6/packages/flutter_tools/lib/src/isolated/devfs_web.dart#L98 - I wonder if we can log a few more details in the error to help us narrow down the problem. Are you able to edit this file in your local main channel branch of flutter and include details like the line and column parameters? I'd expect them both to be 0 since that's what dwds should request when implenting the evaluate API in the VM service procotol.

I also wonder if there are other reasons why here the compilerOutput could be empty (e.g. if the evaluation succeeded but somewhere on the protocol we couldn't transmit the result for some reason).

[sigmundch]

Just to share what I mentioned in https://github.com/flutter/devtools/issues/7766#issuecomment-2163984282, I'd recommend for now skipping the test on web. Unfortunately, the fix I landed only addressed this issue for a subset of the toolchain (our webdev/ddr workflows), but not for the one used by the flutter tool.

Replicating the fix for the flutter tool requires further investigation, as the two systems operate quite differently internally.

[kenzieschmoll]

Just to share what I mentioned in https://github.com/flutter/devtools/issues/7766#issuecomment-2163984282, I'd recommend for now skipping the test on web. Unfortunately, the fix I landed only addressed this issue for a subset of the toolchain (our webdev/ddr workflows), but not for the one used by the flutter tool.

Replicating the fix for the flutter tool requires further investigation, as the two systems operate quite differently internally.

Thanks for the update, Siggi. @adsonpleal lets move forward with disabling this extension for web apps until this issue can be resolved

[kenzieschmoll]

@stuartmorgan I filed https://github.com/flutter/flutter/issues/150210 to find a path forward for building the extension assets on publish of shared_preferences.

[adsonpleal]

@kenzieschmoll I'll work on it this week. I was on vacation and had no access to my computer, but now I'm back and I'll have time to work on this PR.

I have an idea to make it work without the asyncEval. I'll do some experimentations to see if it is possible. If not, we go with the disable for now.

[adsonpleal]

@kenzieschmoll and @stuartmorgan the repo_checks is failing with two errors:

What do you think about removing all the changes in the shared_preferences package? This way the repo_checks will pass. But we'll need to add this file later. Maybe it can be added when resolving this issue.

[kenzieschmoll]

What do you think about removing all the changes in the shared_preferences package? This way the repo_checks will pass. But we'll need to add this file later. Maybe it can be added when resolving this https://github.com/flutter/flutter/issues/150210.

This sounds good to me.

[adsonpleal]

@kenzieschmoll and @stuartmorgan I guess this PR is done, some unrelated tests are failing, though.

[kenzieschmoll]

Before landing this, I think we should discuss how we will be moving forward with https://github.com/flutter/flutter/issues/150210 @stuartmorgan. Otherwise we risk this package sitting here without getting published with shared_preferences.

[stuartmorgan-g]

From triage: I'm currently looking at the blocking issue.

[stuartmorgan-g]

https://github.com/flutter/packages/pull/7156 should unblock this when it lands, since at that point a script can be added here that will do the sub-build-and-copy.

[kenzieschmoll]

#7156 should unblock this when it lands, since at that point a script can be added here that will do the sub-build-and-copy.

Fantastic! So just to verify, once #7156 lands, the next step will be to add a file shared_preferences/tool/pre_publish.dart that does the dart equivalent of the following?

    cd shared_preferences_tool;
    flutter pub get;
    dart run devtools_extensions build_and_copy --source=. --dest=../extension/devtools

[stuartmorgan-g]

I think you're missing a shared_preferences in the dest path, but otherwise that sounds about right to me.