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

Move yew/services to yew-services crate #1693

Merged
merged 12 commits into from
Jan 23, 2021
Merged

Conversation

ranile
Copy link
Member

@ranile ranile commented Jan 10, 2021

Description

Move yew/services to a new yew-services crate. This is one part of the work needed to be done for #1670.

Checklist

  • I have run cargo make pr-flow
    Note: I've had some of the stdweb tests fail, don't know why
  • I have reviewed my own code
  • I have added tests
    Not applicable, only refactoring

Note: I ensured that this code compiles with both web_sys and stdweb but I ran the tests only with web_sys

@siku2 siku2 added the meta Repository chores and tasks label Jan 10, 2021
@github-actions
Copy link

github-actions bot commented Jan 12, 2021

Visit the preview URL for this PR (updated for commit 5e952bb):

https://yew-rs--pr1693-yew-services-grlj554u.web.app

(expires Sat, 30 Jan 2021 13:44:47 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@jstarry
Copy link
Member

jstarry commented Jan 12, 2021

Thanks @hamza1311! I'm wondering if it would be nice to keep the services feature and use it to re-export yew-services at yew::services

@ranile
Copy link
Member Author

ranile commented Jan 12, 2021

@jstarry, that won't work right now because yew-services depends on yew so there will be circular dependencies if yew depends on yew-services

Maybe we could that in a future PR when yew is broken up even more so we can avoid the circular dependency

@jstarry
Copy link
Member

jstarry commented Jan 12, 2021

Ah right, thanks. Yeah that can come later, it probably will make sense to remove yew-services' dependency on yew altogether

# Conflicts:
#	.github/workflows/pull-request.yml
#	packages/yew-services/src/websocket.rs
#	packages/yew-stdweb/Cargo.toml
#	packages/yew/Cargo.toml
#	packages/yew/src/callback.rs
@ranile
Copy link
Member Author

ranile commented Jan 23, 2021

Just bumping this.
I have resolved the conflicts so can this PR be merged now?

@ranile
Copy link
Member Author

ranile commented Jan 23, 2021

Apparently the chrome driver won't start. Any idea what might have gone wrong?

@jstarry
Copy link
Member

jstarry commented Jan 23, 2021

Apparently the chrome driver won't start. Any idea what might have gone wrong?

Hmm, not really sure. I just retried the job

@jstarry
Copy link
Member

jstarry commented Jan 23, 2021

I have resolved the conflicts so can this PR be merged now?

Yeah, let's get it in!

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

There's still quite a bit of cleanup left to do. Maybe we should remove yew-stdweb first?

.github/workflows/pull-request.yml Outdated Show resolved Hide resolved
.github/workflows/pull-request.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
packages/yew/Cargo.toml Outdated Show resolved Hide resolved
packages/yew/Cargo.toml Show resolved Hide resolved
packages/yew/src/callback.rs Outdated Show resolved Hide resolved
packages/yew/Cargo.toml Outdated Show resolved Hide resolved
@ranile
Copy link
Member Author

ranile commented Jan 23, 2021

There's still quite a bit of cleanup left to do.

What you pointed out seems to be because IntelliJ's merge tool somewhat messed up. I fixed up.

Maybe we should remove yew-stdweb first?

I think if we can get this done now, it should happen. Maybe yew-agent and yew-format should happen before yew-stdweb is removed. The next steps (like yew-core as mentioned in #1670, removing yewtil, etc) should happen after we get rid of yew-stdweb. What do you think?

@jstarry
Copy link
Member

jstarry commented Jan 23, 2021

I think if we can get this done now, it should happen.

Ok, great :)

Maybe yew-agent and yew-format should happen before yew-stdweb is removed.

Why is that?

The next steps (like yew-core as mentioned in #1670, removing yewtil, etc) should happen after we get rid of yew-stdweb. What do you think?

Yeah, that sounds good

@ranile
Copy link
Member Author

ranile commented Jan 23, 2021

Maybe yew-agent and yew-format should happen before yew-stdweb is removed.

Why is that?

Because those don't have a specific dependence on yew-stdweb. agent uses in one place and format doesn't use it at all. If it can be done without messing around too much with stdweb now then why not do it? If there's a reason not to, we can delay it.

@jstarry
Copy link
Member

jstarry commented Jan 23, 2021

Because those don't have a specific dependence on yew-stdweb. agent uses in one place and format doesn't use it at all. If it can be done without messing around too much with stdweb now then why not do it? If there's a reason not to, we can delay it.

Ah, I see. It's ok, yew-stdweb removal is almost done anyways

@jstarry
Copy link
Member

jstarry commented Jan 23, 2021

Looks great, @hamza1311! I just added a few small tweaks as well and will merge if CI passes. Thank you!

@jstarry jstarry merged commit ab76a44 into yewstack:master Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Repository chores and tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants