fix: add support for non-root base-href

[holzgeist]

Description

if a web-app is deployed using --base-href=/path/to/deployed/flutter/app during build, the URLs starting with ./assets won't work anymore. Using assetManager will properly resolve them. It uses assetBase from the loader config specified here

NB ⚠️

I only tested the url.startsWith('assets/') code path as it's the only one used in the library. It should work the same for ./ though

[diegotori]

@holzgeist Before I run the workflow, there is one thing that I need you to do.

In other words, we need to move assets/no_sleep.js into the lib folder as per this StackOverflow post. Eventually, it'll be under wakelock_plus/lib/assets/no_sleep.js. The reason being is that it should be moved there so that it's in line with how every other library exposes assets to its users.

You'll also have to modify the existing asset entry in pubspec.yaml so that it points to packages/assets/wakelock_plus/assets/no_sleep.js.

[diegotori]

@holzgeist Please let me know if you're still gonna work on this change. Otherwise, I can commandeer it for you and take it to the finish line. Thanks.

[holzgeist]

Hi @diegotori ,

thanks for the feedback. I'm going to work on it, probably today, maybe tomorrow

[holzgeist]

@diegotori actually I need to solve some other issues before I have a working web-build to test this one. If you could commandeer this, it would be great, thanks 🙏

[diegotori]

@holzgeist please verify the fixes made to this PR. I was able to do a bit of cleanup and/or fixes. Thanks.

[diegotori]

@holzgeist If you have an angle on the latest no_sleep.js changes that I made, which now awaits for the web layer to enable the wakelock before considering it enabled, please feel free to review.

@ditman the above also applies to you as well, if you're able to review the JS changes that I made. Thanks.

[ditman]

I left some comments, but I'm not an actual owner of the plugin, so please, take/ignore as you please from my comments.

Thanks for getting this fix over the finish line, and apologies for the delay in the review, it took me a long while to see this notification!

[ditman]

I know this change is unrelated, but consider adding an id attribute to your script element, that way this method can be simplified to:

return head.querySelector('#$idForScriptElement') != null;

You may need to keep a small map of urls to each of its idForScriptElement String (probably it'll only contain a single element?)

[holzgeist]

done. I hope the locking part works out as planned. These things are really hard to test. The happy path works though

[ditman]

This is better than nothing, but super non-standard! If you want to run tests like this, I'd suggest you try to migrate them to flutter drive, like what we did in flutter/packages, for example:

https://github.com/flutter/packages/tree/main/packages/video_player/video_player_web/example <- this "example" actually contains the integration_test and all that jazz. The README.md should have some links to the docs on how to run those.

[diegotori]

@ditman we already have those flutter drive tests declared in example/integration_test/wakelock_plus_test.dart. I guess the question is whether these are the tests that should be run instead of the above ones?

[ditman]

Right, we also have some flutter test --platform=chrome tests in flutter/packages.

I'd recommend using either flutter test or flutter drive to run the tests. I'm not even sure what this program would return if the tests failed? (is this usable for CI, for example?)

[diegotori]

@ditman Looks like even after porting this test to use flutter drive, it's still complaining about the requesting page not being visible.

@TestOn('browser')
library;

import 'package:flutter_test/flutter_test.dart';
import 'package:integration_test/integration_test.dart';
import 'package:wakelock_plus/src/wakelock_plus_web_plugin.dart';
import 'package:wakelock_plus/wakelock_plus.dart';
// ignore: depend_on_referenced_packages
import 'package:wakelock_plus_platform_interface/wakelock_plus_platform_interface.dart';

///
/// Run these tests with:
///   flutter run -d chrome test/wakelock_plus_web_plugin_test.dart
///
void main() {
  IntegrationTestWidgetsFlutterBinding.ensureInitialized();
  group('$WakelockPlusWebPlugin', () {
    setUpAll(() async {
      WakelockPlusPlatformInterface.instance = WakelockPlusWebPlugin();
    });

    tearDown(() async {
      await WakelockPlus.disable();
    });

    testWidgets('$WakelockPlusWebPlugin set as default instance',
        (tester) async {
      expect(
          WakelockPlusPlatformInterface.instance, isA<WakelockPlusWebPlugin>());
    });

    testWidgets('initially disabled', (tester) async {
      expect(WakelockPlus.enabled, completion(isFalse));
    });

    testWidgets('enable', (tester) async {
      await WakelockPlus.enable();
      expect(WakelockPlus.enabled, completion(isTrue));
    });

    testWidgets('enable more than once', (tester) async {
      await WakelockPlus.enable();
      await WakelockPlus.enable();
      await WakelockPlus.enable();
      expect(WakelockPlus.enabled, completion(isTrue));
    });

    testWidgets('disable', (tester) async {
      await WakelockPlus.enable();
      await WakelockPlus.disable();
      expect(WakelockPlus.enabled, completion(isFalse));
    });

    testWidgets('disable more than once', (tester) async {
      await WakelockPlus.enable();
      await WakelockPlus.disable();
      await WakelockPlus.disable();
      await WakelockPlus.disable();
      expect(WakelockPlus.enabled, completion(isFalse));
    });

    testWidgets('toggle', (tester) async {
      await WakelockPlus.toggle(enable: true);
      expect(WakelockPlus.enabled, completion(isTrue));

      await WakelockPlus.toggle(enable: false);
      expect(WakelockPlus.enabled, completion(isFalse));
    });
  });
}

Run with while in the example folder:

flutter drive -d web-server --web-port 7357 --browser-name chrome --driver test_driver/integration_test.dart --target integration_test/wakelock_plus_web_plugin_test.dart

[diegotori]

Bottom line, what I'm looking for is an integration test that runs on a browser, that can also be spun up in a CI context. Not sure which one is the right approach, since the above is not working at all when using chromedriver.

[diegotori]

@ditman looks like it's related to this issue when running flutter test --platform=chrome on wakelock_plus_web_plugin_test.dart in the root test folder.

Not sure what can be done if asset support isn't present when running the tests against a browser.

[ditman]

I suspect the problem is that the WakelockPlus calls must happen in the application under test, and not the test code itself.

If you inject a test app that has a button that enables/disables wakelock, and you trigger the button through the test driver, this should work, and assets would be available.

I think the code here is attempting to run Wakelock in the test runner code, not the application under test? :)

[ditman]

(I don't have time to try my solution right now, but I think the migration to an integration test with flutter driver might be inescapable if we want to test app assets)

[diegotori]

So what you're saying is that I should instead inject the Example App, get a lock on the button that toggles the wakelock, and then make assertions from there?

[ditman]

Yes, that's what I'm saying... What I'm not sure now is that the assets are going to be available when the example app runs in flutter drive mode, as you're saying (because your test looks all right to me).

As a reference, a few examples of tests that "inject an app and then look at the DOM of the page or something else" to do integration testing:

• https://github.com/flutter/packages/blob/main/packages/url_launcher/url_launcher_web/example/integration_test/link_widget_test.dart
• https://github.com/flutter/packages/blob/main/packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/projection_test.dart

Neither of those use assets, though.

Here's an example of an integration test that uses assets:

• https://github.com/flutter/packages/blob/main/packages/google_maps_flutter/google_maps_flutter_web/example/integration_test/markers_test.dart#L251-L254

[holzgeist]

@diegotori thanks for working on this. But unfortunately this grew way outside of my comfort zone wrt flutter/web so I have to pass on reviewing this 🙈

[diegotori]

I left some comments, but I'm not an actual owner of the plugin, so please, take/ignore as you please from my comments.

Thanks for getting this fix over the finish line, and apologies for the delay in the review, it took me a long while to see this notification!

Thanks for reviewing this. Since you have a better handle on the JS side of things, I'll let you take point on what I should modify. Feel free to call out more things as you see them.

[holzgeist]

Hi there!
I'm back to actively working on a flutter/web port of our app. Is there anything I can help with on this PR?

[holzgeist]

ok, thanks to @ditman 's super detailed review and suggestions I actually was able to understand everything that's going on and apply the suggestions (apart from testing). They definitely all look and feel like improvements, thanks again for the through review 🙏

@diegotori @ditman any thoughts?

[diegotori]

@holzgeist I'll definitely take a look hopefully this week.

Thanks for your contributions thus far.

[diegotori]

@holzgeist so far, it LGTM. @ditman, any thoughts on his changes?

[ditman]

Never meant to block this code, thanks for all the improvements!

I don't have time to actually run this code, but if users and CI are happy with it, I don't have much more to say! :)

LGTM!

[ditman]

Right, we also have some flutter test --platform=chrome tests in flutter/packages.

I'd recommend using either flutter test or flutter drive to run the tests. I'm not even sure what this program would return if the tests failed? (is this usable for CI, for example?)

[holzgeist]

I added a commit to prevent _nativeEnabledCompleter from being null in the catch branch. Since then I realized that I had two requests running in parallel which interfered with each other. I'll still leave the commit there because it doesn't hurt :)