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

allow web worker resource to be relative #2086

Merged
merged 8 commits into from Nov 20, 2021

Conversation

astraw
Copy link
Contributor

@astraw astraw commented Sep 26, 2021

Description

This PR allows a web worker resource (the .js and _bg.wasm files) to be specified relative to the loading path. Prior to this PR, the resource is specified with an absolute path. In this case - with an absolute path - if the resource is specified at "location.js", the path "/location.js" will be used.

As discussed in #2056, this adds a new method name_of_resource_is_relative to the trait yew_agent::Agent. The default implementation returns false and the current behavior - to specify an absolute path - remains unchanged. What is new here is that if an implementation implements this method and returns true, the location of a web worker script is interpreted as relative to the current path.

Fixes #2056.

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code

@mc1098 mc1098 added the A-yew-agent Area: The yew-agent crate label Sep 26, 2021
@astraw
Copy link
Contributor Author

astraw commented Sep 26, 2021

I didn't notice the MSRV of 1.49. I'll fix this.

Copy link
Contributor

@mc1098 mc1098 left a comment

Choose a reason for hiding this comment

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

A suggestion from me regarding re-implementing a std fn not yet available in MSRV.
Also admission that your naming is better than mine :)

packages/yew-agent/src/worker/mod.rs Outdated Show resolved Hide resolved
packages/yew-agent/src/worker/private.rs Outdated Show resolved Hide resolved
packages/yew-agent/src/worker/public.rs Outdated Show resolved Hide resolved
packages/yew-agent/src/worker/mod.rs Outdated Show resolved Hide resolved
packages/yew-agent/src/worker/mod.rs Outdated Show resolved Hide resolved
packages/yew-agent/src/lib.rs Outdated Show resolved Hide resolved
@lausek
Copy link
Contributor

lausek commented Nov 8, 2021

Hi @astraw, could you please review the suggested changes to make this PR mergeable?

@voidpumpkin voidpumpkin added the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Nov 11, 2021
@astraw
Copy link
Contributor Author

astraw commented Nov 20, 2021

I think this should be good to merge. However, Github shows me "Changes requested" although I thought I have now implemented all requested changes as far as I can see. Probably it is something with the Github PR UI that I do not understand, as I am not very familiar with it. Anyhow, it would be great if you would check this again and merge if you find appropriate.

(Apologies for the delay, the last weeks have been busy.)

@mc1098 mc1098 removed the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Nov 20, 2021
@mc1098
Copy link
Contributor

mc1098 commented Nov 20, 2021

Github shows me "Changes requested" although I thought I have now implemented all requested changes as far as I can see.

It doesn't change until the person who made the request for changes approves of those changes :)

Thank you for changes 🎉 LGTM.

@voidpumpkin second pair of eyes please 👀

@voidpumpkin voidpumpkin merged commit 916dc2e into yewstack:master Nov 20, 2021
@astraw astraw deleted the specify-web-workers-as-relative branch November 20, 2021 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-agent Area: The yew-agent crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow specifying worker script location as absolute or relative to current path
4 participants