Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[path_provider] Move Windows FFI behind a conditional import #3056

Merged

Conversation

stuartmorgan
Copy link
Contributor

@stuartmorgan stuartmorgan commented Sep 20, 2020

Description

Moves the real implementation of path_provider_windows behind a
conditional export, instead exporting a stub on platforms that don't
support dart:ffi. This avoids build breakage in web projects that have
transitive dependencies on path_provider (and thus path_provider_windows
due to manual endorsement).

This will no longer be necessary once
flutter/flutter#52267 is fixed, since only
Windows builds will ever need to have code-level dependency on
path_provider_windows.

Related Issues

Fixes flutter/flutter#66143

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • 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.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • 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.

Moves the real implementation of path_provider_windows behind a
conditional export, instead exporting a stub on platforms that don't
support dart:ffi. This avoids build breakage in web projects that have
transitive dependencies on path_provider (and thus path_provider_windows
due to manual endorsement).

This will no longer be necessary once
flutter/flutter#52267 is fixed, since only
Windows builds will ever need to have code-level dependency on
path_provider_windows.
@stuartmorgan
Copy link
Contributor Author

The manual registration workaround for flutter/flutter#52267 is truly the gift that keeps on giving (pain).

Two notes:

  • The diffing is terrible here; path_provider_windows_real.dart is just the original file renamed, and with the import of folder.dart fixed.
  • I considered doing this stubbing at the path_provider level, conditionally importing either the path_provider_windows package or a stub, since that's where the actual problem is, but realized that it would be a land-mine. Any package that depended directly on path_provider_windows would then transitively export the same problem--and that's not a theoretical problem, because shared_preferences_windows will do exactly that (that's done instead of depending on path_provider, because I don't want, say, iOS projects using shared_preferences but not otherwise using path_provider to get a native plugin compiled in that they don't actually need). So while this is a little uglier, it restricts the ugliness to this plugin.

Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

@stuartmorgan stuartmorgan merged commit 2595703 into flutter:master Sep 21, 2020
@stuartmorgan stuartmorgan deleted the path-provider-windows-conditional-ffi branch September 21, 2020 20:48
@joe-getcouragenow
Copy link

@gspencergoog or anyone els that knows...

How i can see if this has hit the flutter master channel ?

@stuartmorgan
Copy link
Contributor Author

How i can see if this has hit the flutter master channel ?

Plugins aren't part of the Flutter framework.

@timsneath
Copy link
Contributor

@joe-getcouragenow Just to add a little here, I can see the potential for confusion, since GitHub reports this as "merged commit 2595703 into flutter:master. But as Stuart mentions, this is part of the plugins repo.

If you look at the commit, you can see that it's 0.0.4, and the version of path_provider_windows published is 0.0.4+1, so it looks like you can be confident that you have the right build if you run pub upgrade.

danielroek pushed a commit to Baseflow/flutter-plugins that referenced this pull request Oct 1, 2020
…#3056)

Moves the real implementation of path_provider_windows behind a
conditional export, instead exporting a stub on platforms that don't
support dart:ffi. This avoids build breakage in web projects that have
transitive dependencies on path_provider (and thus path_provider_windows
due to manual endorsement).

This will no longer be necessary once
flutter/flutter#52267 is fixed, since only
Windows builds will ever need to have code-level dependency on
path_provider_windows.
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
…#3056)

Moves the real implementation of path_provider_windows behind a
conditional export, instead exporting a stub on platforms that don't
support dart:ffi. This avoids build breakage in web projects that have
transitive dependencies on path_provider (and thus path_provider_windows
due to manual endorsement).

This will no longer be necessary once
flutter/flutter#52267 is fixed, since only
Windows builds will ever need to have code-level dependency on
path_provider_windows.
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
…#3056)

Moves the real implementation of path_provider_windows behind a
conditional export, instead exporting a stub on platforms that don't
support dart:ffi. This avoids build breakage in web projects that have
transitive dependencies on path_provider (and thus path_provider_windows
due to manual endorsement).

This will no longer be necessary once
flutter/flutter#52267 is fixed, since only
Windows builds will ever need to have code-level dependency on
path_provider_windows.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
5 participants