Skip to content

Conversation

@cfiorillo
Copy link
Contributor

Description

When accessing the version.json file the generated URL contains an extra forward-slash, that can prevent in some deployments access to the file

Related Issues

#96

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I am willing to follow-up on review comments in a timely manner.

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 a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@miquelbeltran
Copy link
Member

Thanks @cfiorillo !

Is there a way to have a test for this issue?

Did you run the e2e test for this plugin?

@mhadaily mhadaily added the package_info_plus stuff related to package_info_plus label Jan 28, 2021
@cfiorillo
Copy link
Contributor Author

cfiorillo commented Jan 28, 2021

Hi @miquelbeltran
I tried to run the e2e test, but:

  1. there are no e2e tests on the package_info_plus_web defined
  2. the e2e tests on the package_info_plus fails also if I run them on the main branch.

Regarding point 2, trying the e2e tests on package_info_plus the "expect" for web are wrong; this code (in package_info_plus_e2e.dart file)

   } else if (kIsWeb) {
      expect(info.appName, 'package_info_example');
      expect(info.buildNumber, null);
      expect(info.packageName, null);
      expect(info.version, null);
    } else {

should be replaced by

    } else if (kIsWeb) {
      expect(info.appName, 'package_info_example');
      expect(info.buildNumber, '4');
      expect(info.version, '1.2.3');
    } else {

since the pubspec.yaml file contains:

version: 1.2.3+4

Also, running the above tests on a Mac seems like the

if (Platform.isMacOS)

is verified (=true) also if I'm running the web tests with the following command line:

flutter drive --target=./test_driver/package_info_plus_e2e.dart --release -d chrome

@miquelbeltran
Copy link
Member

there are no e2e tests on the package_info_plus_web defined

That's OK, the projects are structured in a way all e2e tests are on the root project :)

the e2e tests on the package_info_plus fails also if I run them on the main branch.

That's good to know at least, thanks for checking! I am aware that not all e2e test pass on all platforms, so I will fill a ticket for that.

Copy link
Member

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

I was going to ask if we should ensure that there's always a single slash between in case the base URI wouldn't end with a slash, but Flutter's index.html clearly states that the base URI must end with a slash, so this is ok.

  <!--
    If you are serving your web app in a path other than the root, change the
    href value below to reflect the base path you are serving from.

    The path provided below has to start and end with a slash "/" in order for
    it to work correctly.

    Fore more details:
    * https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base
  -->
  <base href="/">

@cfiorillo
Copy link
Contributor Author

Thanks for the fix!

I was going to ask if we should ensure that there's always a single slash between in case the base URI wouldn't end with a slash, but Flutter's index.html clearly states that the base URI must end with a slash, so this is ok.

  <!--
    If you are serving your web app in a path other than the root, change the
    href value below to reflect the base path you are serving from.

    The path provided below has to start and end with a slash "/" in order for
    it to work correctly.

    Fore more details:
    * https://developer.mozilla.org/en-US/docs/Web/HTML/Element/base
  -->
  <base href="/">

HI @jpnurmi, while preparing the PR I initially added the slash before version.json only if there was no trailing slash in window.document.baseUri, but I read the comment above in index.html and changed mind ;-)

Thanks for the approval, cool

@jpnurmi jpnurmi merged commit a949352 into fluttercommunity:main Jan 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

package_info_plus stuff related to package_info_plus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants