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

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

[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 :)

[james-pellow]

I'm needing this as well. Any progress on it?

[diegotori]

@ditman looks like flutter drive was the winner, since I'm able to spin that up in GH Actions. Granted, although it hangs after all the Chrome integration tests pass, I set a timeout for that command in the action.

Not sure what you guys do over there to handle that situation. If you're able to shed some light on the current web_integration_test config, that would be awesome. Nevertheless, if Chrome says that the tests are passing when running the plugin through that platform, then it's good and dandy in my book. I'll probably release this once the tests go green.

[diegotori]

LGTM again.

[diegotori]

@holzgeist and @ditman thanks again for your input on this PR.

This will get merged and deployed very shortly.

[ditman]

@diegotori thanks for pushing this all the way through! This has been a long time coming :)

If you're able to shed some light on the current web_integration_test config, that would be awesome.

I was checking your command-line arguments to flutter drive, and comparing them to Flutter's; this is what we have that is web specific:

• https://github.com/flutter/packages/blob/main/script/tool/lib/src/drive_examples_command.dart#L158-L167

And here's where the code is actually run:

• https://github.com/flutter/packages/blob/main/script/tool/lib/src/drive_examples_command.dart#L444-L456

(Same file, it's... big)

Anyway, the only difference I see is that the infra is different, you use github actions but flutter/plugins uses google's LUCI infra. Our tests are pinning the flutter version and the Chrome version (not sure if that makes a difference in this particular case).

In my experience, I've seen tests hang in Chrome when things don't complete, or something odd happens. Have you tried to run a super lame integration test in chrome that doesn't have any async code (literally just "expect(true, isTrue / isFalse)" or similar) to verify whether your issue is in the infra, or in the test itself?

Debugging hanging drive tests is a bit of a chore, I normally comment out most of the test until I get to the conflictive line(s), and work from there. I've been affected many times by tests that await somethingThatNeverResolves(...), and it'll just hang, unfortunately!