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

feat(package_info_plus)!: Support multiple version.json locations #2733

Merged
merged 9 commits into from
Mar 23, 2024

Conversation

AriasBros
Copy link
Contributor

@AriasBros AriasBros commented Mar 19, 2024

Description

Currently, the version.json file is fetched using a web specific API to get the base URI. Because the base URI obtained with this API is always the URL from the browser, this could lead that some setups where the Flutter app is deployed and served using different domains and paths could fail.

This PR changes that replacing the web specific API with a Flutter API to get the right URL used to load the assets of the application.

Related Issues

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I titled the PR using Conventional Commits.
  • I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files.
  • All existing and new tests are passing.
  • The analyzer (flutter analyze) does not report any problems on my PR.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
  • No, this is not a breaking change.

@miquelbeltran
Copy link
Member

Thanks for the pr! I was experimenting with the same yesterday, it's kind of weird that the framework doesn't provide a direct way to obtain the base URLs. As pointed out, reading the base URL from the browser window is not exactly right in all cases.

My concern is if this would be wrong, given the case a user setups a different assets' URL than the entry point where the app is installed.

e.g.

_flutter.loader.loadEntrypoint({
    entrypointUrl: "https://www.app-domain-1.com/app/main.dart.js"
    onEntrypointLoaded: async function(engineInitializer) {
        let appRunner = await engineInitializer.initializeEngine({
            assetBase: "https://www.app-domain-2.com/myappcontent/"
        });
        appRunner.runApp();
    }
});

Where would we expect the version.json to be found? in domain 1 or 2?

So, we can have three URLs:

  • The browser URL
  • The entry point URL (where the Dart JS code is)
  • The Assets URL

Another option is to use it as fallback, say, check if the version.json exists on assets base URL, if not, check on browser URL. This could be done together with also providing a custom param.

Allowing to do:

  1. If a custom version.json path exists, use that.
  2. If not, check in the assets' base URL.
  3. If not, use browser URL.

@miquelbeltran
Copy link
Member

miquelbeltran commented Mar 22, 2024

Hi @AriasBros I am thinking on taking your commit 860b8ba and creatting a new PR that would support custom version.json url + fallback to browser baseUrl.

Do you mind if I get your commit and continue adding to that?

@AriasBros
Copy link
Contributor Author

Hi @miquelbeltran,

Sorry the delay to answer you. I agree with your proposed solution, but these days my company changed my priorities and this one was down a little in the list, but I think I will reach this task today in a few hours.

I would like to send an update to this PR today or the monday.

If there is hurry to fix this issue just let me know and in that case of course you can use my commit, no problem with that.

@miquelbeltran

This comment was marked as outdated.

@miquelbeltran miquelbeltran changed the title fix(package_info_plus): Use the AssetManager to get the base URL feat(package_info_plus): Support custom version.json location, use the AssetManager to get the base URL, or fallback to browser base URL Mar 22, 2024
@miquelbeltran miquelbeltran changed the title feat(package_info_plus): Support custom version.json location, use the AssetManager to get the base URL, or fallback to browser base URL fix(package_info_plus): Use the AssetManager to get the base URL Mar 22, 2024
@miquelbeltran
Copy link
Member

miquelbeltran commented Mar 22, 2024

Actually, I will just restore back to the original state of your PR, I will experiment on my own branch: miquelbeltran/2732

Copy link
Member

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far the implementation is looking good.

I've left a question regarding the custom baseUrl.

I'll do some tests when I got the time.

We also should add integration tests. Testing the custom version.json uri should be easy (like done here:

testWidgets(
'Get package info using custom version json uri',
(tester) async {
when(client.get(customUri)).thenAnswer(
(_) => Future.value(
http.Response(jsonEncode(VERSION_JSON), 200),
),
);
final versionMap = await plugin.getAll(customVersionJson: customUri);
expect(versionMap.appName, VERSION_JSON['app_name']);
expect(versionMap.version, VERSION_JSON['version']);
expect(versionMap.buildNumber, VERSION_JSON['build_number']);
expect(versionMap.packageName, VERSION_JSON['package_name']);
expect(versionMap.buildSignature, VERSION_JSON['build_signature']);
},
);
). Testing the assets' manager may not be as easy without allowing to mock it from the outside, but it is something we can do at the end.

@AriasBros
Copy link
Contributor Author

We also should add integration tests. Testing the custom version.json uri should be easy. Testing the assets' manager may not be as easy without allowing to mock it from the outside, but it is something we can do at the end.

Yes, it is the next thing I want to work on.

@miquelbeltran miquelbeltran changed the title fix(package_info_plus): Use the AssetManager to get the base URL feat(package_info_plus)!: Support multiple version.json locations Mar 23, 2024
@miquelbeltran
Copy link
Member

I'll mark the PR as breaking, just because the platform interface signature also changed, and I want to avoid version conflicts when pulling dependencies. Technically, this could pass as a non-breaking for package users, but I want to be on the safe side.

Copy link
Member

@miquelbeltran miquelbeltran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, David! I think this is good to go for now. Let me know if you still want to do some more changes.

The build.yaml was added to be able to create the AssetManager mock. See more here: https://stackoverflow.com/a/68275812
@AriasBros
Copy link
Contributor Author

Thank you, David! I think this is good to go for now. Let me know if you still want to do some more changes.

Thanks @miquelbeltran

From my side I don't see nothing else to do. Maybe update online documentation, but I am not sure if I should do it in this same PR or that follows other procedures. Just let me know about it, I am available for any change that this PR could need.

@AriasBros
Copy link
Contributor Author

I'll mark the PR as breaking, just because the platform interface signature also changed, and I want to avoid version conflicts when pulling dependencies. Technically, this could pass as a non-breaking for package users, but I want to be on the safe side.

If it is necessary, I could split this PR in 2:

  • One adding only the implementation about to use the AssetManager base URL, which it should not be a breaking change.
  • And then other with the changes related to add the custom base URL parameter.

Anyway, in this last commit that I sent, I added a new parameter to the plugin constructor to allow testing the AssetManager. I understand that that constructor is a private API of the package, but well, maybe it could be considered a breaking change too.

@miquelbeltran
Copy link
Member

Thanks for including a test with the AssetManager as well!

Having a breaking change is not an issue, it will simply go into a new major version when we generate a new release, which I prefer anyway. It may take a while until we release the package since we did a big release very recently, but this will be available to anyone who wants to try using a git ref dependency.

I'm specially interested if people in #2699 can give it a try.

Thanks again for the work! I will merge now, any further improvements we can do in a new PR.

@miquelbeltran miquelbeltran merged commit 26047f3 into fluttercommunity:main Mar 23, 2024
19 of 21 checks passed
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.

[Bug]: package_info_plus is not using the right URL to load the version.json file in web
2 participants