[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.

shared_preferences_tools.mp4

Pre-launch Checklist

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

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

[kenzieschmoll]

The extension/devtools/build folder will need to either be checked in, or the devtools_extensions build_and_copy command to generate the extension assets from the source code will need to be included as part of the pub publish flow for shared_preferences. The latter would be more safe since the machine doing the publish would also be the one to build the extension assets. However, the extension likely will not need to be regenerated for each publish of shared_preferences - only if there are updates to the extension. So that should be taken into consideration.

@stuartmorgan do you have a preference or some guidance on this?

[stuartmorgan]

The extension/devtools/build folder will need to either be checked in, or the devtools_extensions build_and_copy command to generate the extension assets from the source code will need to be included as part of the pub publish flow for shared_preferences. The latter would be more safe since the machine doing the publish would also be the one to build the extension assets.

We definitely don't want to introduce new processes that involve individuals building precompiled binaries on uncontrolled machines and then checking them in.

However, the extension likely will not need to be regenerated for each publish of shared_preferences - only if there are updates to the extension. So that should be taken into consideration.

How long does that build take?

[kenzieschmoll]

all the new dart files in this PR need the Flutter copyright:

// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

[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?

[stuartmorgan]

The extension/devtools/build folder will need to either be checked in, or the devtools_extensions build_and_copy command to generate the extension assets from the source code will need to be included as part of the pub publish flow for shared_preferences. The latter would be more safe since the machine doing the publish would also be the one to build the extension assets.

We definitely don't want to introduce new processes that involve individuals building precompiled binaries on uncontrolled machines and then checking them in.

However, the extension likely will not need to be regenerated for each publish of shared_preferences - only if there are updates to the extension. So that should be taken into consideration.

How long does that build take?

[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]

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]

@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]

This looks great! Done with first round of review on the extension content itself.

[kenzieschmoll]

@stuartmorgan is there a policy to follow with respect to checking in generated mock files?

[adsonpleal]

@stuartmorgan I've set the same owner as the root shared_preferences folder.

[stuartmorgan]

@kenzieschmoll Was the intent of the issue that the ecosystem take ownership of this?

Adding several thousand lines of code that we don't have much context on, and which are fundamentally different from code the ecosystem team has experience maintaining, is a substantial long-term eng burden that would require a higher level discussion.

[kenzieschmoll]

From my perspective, the intent was that @adsonpleal would continue to own this extension, just as they would have if it continued to live under their own Github repository (https://github.com/adsonpleal/shared_preferences_tools).

The value add of moving this tool to the flutter/packages repo and distributing along with the companion package shared_preferences was to reach and provide value to all shared_preferences users by tightly coupling this tool with its target package.

That being said, if breaking changes were to be made to shared_preferences that would break this companion tool, folks contributing to shared_preferences should be aware of those implications and should work with @adsonpleal to ensure the shared_preferences tool continues to work as intended.

[stuartmorgan]

In that case let's list Tarrin, but put a comment on the line saying that @adsonpleal is the primary maintainer. Longer term we can change the owner line, but that requires commit access.

[adsonpleal]

Perfect! Gonna add a comment mentioning this.

[adsonpleal]

@kenzieschmoll and @stuartmorgan is this enough?

[kenzieschmoll]

Looks good to me, but I will let @stuartmorgan confirm. Nit: typo in "maintainer"

[stuartmorgan]

Let's add "but is not yet a committer, so can't be listed as the owner."

[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: dart-lang/sdk@9dad32c. @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 flutter/devtools#7766 (comment), 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 flutter/devtools#7766 (comment), 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 flutter/flutter#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 flutter/flutter#150210.

This sounds good to me.

[adsonpleal]

@kenzieschmoll this is the workaround that I've mentioned. Basically I'm getting the SharedPreferences.getInstance() value without calling the asyncEval. I took inspiration on the asyncEval implementation, but replaced the postEvent call with an hack to save the value on a list (just to get the value by reference).

Let me know what you think, if you think this is too much of a hack I can revert the commits related to this change and we release without the web version for now.

[kenzieschmoll]

can you use Object? here?

[kenzieschmoll]

can you link the bug here?

[kenzieschmoll]

A couple comments but the workaround LGTM

[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 flutter/flutter#150210 @stuartmorgan. Otherwise we risk this package sitting here without getting published with shared_preferences.