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

[dom] Update Snabbdom to v3 #972

Merged
merged 1 commit into from
Jul 4, 2021
Merged

[dom] Update Snabbdom to v3 #972

merged 1 commit into from
Jul 4, 2021

Conversation

ryota-ka
Copy link
Contributor

Addresses #944

After this pull request is merged, Snabbdom v0.x will no longer be supported.
So a major version bump is required.

  • There exists an issue discussing the need for this PR
  • I added new tests for the issue I fixed or built
  • I used pnpm run commit instead of git commit

@ryota-ka
Copy link
Contributor Author

Probably this would also address #925.

Copy link
Member

@jvanbruegge jvanbruegge left a comment

Choose a reason for hiding this comment

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

Looks good, basically just changing the imports. Exactly what I expected for the move to v3. That's nice to see.
Just one small nit pick, but LGTM

import xs, {Stream, Listener} from 'xstream';
import concat from 'xstream/extra/concat';
import sampleCombine from 'xstream/extra/sampleCombine';
import {DOMSource} from './DOMSource';
import {MainDOMSource} from './MainDOMSource';
import {VNode} from 'snabbdom/vnode';
import {toVNode} from 'snabbdom/tovnode';
import {VNode,toVNode} from 'snabbdom';
Copy link
Member

Choose a reason for hiding this comment

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

Could you move all the imports into a single statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged into line 2! Thanks!

BREAKING CHANGE: 🧨 Snabbdom v0.x is no longer supported.

✅ Closes: cyclejs#944
@jvanbruegge
Copy link
Member

I've checked the tests myself and everything still works as expected.
@staltz if this looks good to you, can you merge and release?

@staltz
Copy link
Member

staltz commented Jul 4, 2021

Yes, l'll do that

@staltz staltz merged commit 2a80dce into cyclejs:master Jul 4, 2021
@ryota-ka ryota-ka deleted the snabbdom@3 branch July 4, 2021 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants