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 specifying worker script location as absolute or relative to current path #2056

Closed
1 of 3 tasks
Tracked by #2052
astraw opened this issue Sep 15, 2021 · 3 comments · Fixed by #2086
Closed
1 of 3 tasks
Tracked by #2052

Allow specifying worker script location as absolute or relative to current path #2056

astraw opened this issue Sep 15, 2021 · 3 comments · Fixed by #2086
Assignees
Labels
A-yew-agent Area: The yew-agent crate feature-accepted A feature request that has been accepted and needs implementing feature-request A feature request

Comments

@astraw
Copy link
Contributor

astraw commented Sep 15, 2021

Problem
In older yew (0.15 and earlier), worker scripts were specified with a name relative to the main script. Now, I must anticipate the final URL of my scripts and specify any leading path components in Agent::name_of_resource() because this is now treated as relative to the origin and thus absolute. However, it would be useful to allow the relative behavior again as this would allow yew apps (with workers) to be dropped into a directory not at the root of a website,

One could potentially call this a regression but the behavior was purposefully changed in #1208 to fix #1175. In #1175, @domdir made two proposals: href should be replaced with either: "page origin" or "page origin + pathname". As noted (While page origin + pathname is closer to the way it behaves currently, I think the correct solution, however, is to only use the page origin), the preference was for "page origin" only and that was indeed the result of #1208, but I think "page origin + pathname" has a valid usecase.

Expected behavior
I think it should be possible for an app to opt-in to the old behavior or specifying the web worker name relative to the app.

Environment:

  • Yew version: 0.16 and later

Questionnaire

  • I'm interested in fixing this myself but don't know where to start
  • I would like to fix and I can work on a solution but this requires discussion
  • I don't have time to fix this right now, but maybe later
@astraw astraw added the bug label Sep 15, 2021
astraw added a commit to strawlab/strand-braid that referenced this issue Sep 15, 2021
Because the computation is actually really fast and due to
yewstack/yew#2056, it is easier to simply not
use threads to handle this computation.
@mc1098 mc1098 added the A-yew-agent Area: The yew-agent crate label Sep 20, 2021
@mc1098
Copy link
Contributor

mc1098 commented Sep 21, 2021

Hi 👋
Thank you for the issue and all the time you've spent looking into when this change occurred and sorry for the delay in response.

it would be useful to allow the relative behavior again as this would allow yew apps (with workers) to be dropped into a directory not at the root of a website

I think "page origin + pathname" has a valid usecase.

Agreed, I think the use case you stated above shows perfectly why this would be the case. I'd like to see both approaches available for users.

I would like to fix and I can work on a solution but this requires discussion

I'm curious whether you had a possible solution in mind?

Housekeeping:
I'm going to remove the bug label and add a feature-request label.

One could potentially call this a regression but the behavior was purposefully changed in #1208 to fix #1175.

As you've stated the change was purposeful so bug doesn't seem correct to me, even though I agree that it actually results in more restrictive behaviour. I also think it's a feature request because, I think we agree that, just reverting the change isn't a "fix" and I think we can do something better :)

If you think this is wrong just let me know 👍.

@mc1098 mc1098 added feature-request A feature request and removed bug labels Sep 21, 2021
@astraw
Copy link
Contributor Author

astraw commented Sep 21, 2021

Thanks, I agree with all you write.

Regarding a proposed solution, how about adding the following method and default implementation to the Agent trait:

/// Declares the behavior of the agent.
pub trait Agent: Sized + 'static {

    // ...

    fn resource_path_is_relative() -> bool {
        false
    }
}

This would allow setting an relative path if desired in the implementation of the trait, but would keep the current behavior as default.

I haven't implemented this but it seems like it should work.

@mc1098
Copy link
Contributor

mc1098 commented Sep 22, 2021

I'm happy with this approach, though I'd prefer if the name of the function was more similar to name_of_resource so it's abundantly clear that the bool is in reference to this.

Marking this as feature accepted so feel to submit a PR with this change 🎉Any issues just ping me either here or in the PR :)

@mc1098 mc1098 added the feature-accepted A feature request that has been accepted and needs implementing label Sep 22, 2021
astraw added a commit to astraw/yew that referenced this issue Sep 26, 2021
astraw added a commit to astraw/yew that referenced this issue Sep 26, 2021
astraw added a commit to astraw/yew that referenced this issue Sep 26, 2021
voidpumpkin pushed a commit that referenced this issue Nov 20, 2021
* allow web worker resource to be relative

fixes #2056

* make backwards compatible with rust < 1.52

* fix clippy warning

* Apply suggestions from code review

Co-authored-by: mc1098 <m.cripps1@uni.brighton.ac.uk>

* do not reimplement stdlib function

* fixup previous

* improve docstring

Co-authored-by: mc1098 <m.cripps1@uni.brighton.ac.uk>
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 feature-accepted A feature request that has been accepted and needs implementing feature-request A feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants