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

Migrate to web-sys #32

Merged
merged 10 commits into from
Sep 18, 2018
Merged

Migrate to web-sys #32

merged 10 commits into from
Sep 18, 2018

Conversation

chinedufn
Copy link
Owner

@chinedufn chinedufn commented Sep 18, 2018

What

This PR migrates virtual-dom-rs to use web-sys and deletes the percy-webapis crate.

Why

This was always the plan, we were just waiting for web-sys to mature.

This PR makes it easier for people to use web-sys alongside Percy. And just generally means that we've staying in line with the wasm-bindgen project.

Highlights

  • We were previously being a bit hacky and turning things into HtmlElements that really aren't elements. web-sys enforces things only being able to be type casted in ways that are actually specified in web-idl, so we had to refactor some of our virtual dom code a bit. No major overhauls, just being a bit more correct in a few places

  • Added some new browser tests to our jsdom test suite. A TODO is to deprecate this suite in favor of using wasm-bindgen-test in chrome and firefox

  • Added some documentation on how we handle text nodes

  • Overall there isn't any new functionality here.. Just some re-jigging to use the real types that we're supposed to use instead of making things that aren't HtmlElement into HtmlElement like we were before.

Closes #24

@vercel
Copy link

vercel bot commented Sep 18, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@qm3ster
Copy link
Contributor

qm3ster commented Sep 19, 2018

Sorry, why is this merged? I think there's a compile error on master if I do start.sh right now.

@chinedufn
Copy link
Owner Author

mmmmm it was working locally for me my bad!! taking a look

@chinedufn
Copy link
Owner Author

@qm3ster what error are you seeing? I don't see anything. Hmm..

@chinedufn
Copy link
Owner Author

I ran cargo update and now my jsdom tests fail but the demo still works for me.

I'll fix the jsdom tests

@qm3ster
Copy link
Contributor

qm3ster commented Sep 19, 2018

Currently on master:

thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', /home/mihail/.cargo/registry/src/github.com-1ecc6299db9ec823/wasm-bindgen-cli-support-0.2.21/src/descriptor.rs:277:15
note: Run with `RUST_BACKTRACE=1` for a backtrace.

This is wasm-bindgen.
I do think it's a different error from what I saw yesterday.

On example/isomorphic branch it's fine.

Does it really work for you?
What is your platform and what wasm-bindgen -V?

@chinedufn
Copy link
Owner Author

Yeah I can build/run the isomorphic app just fine on wasm-bindgen 0.2.21

I get the same error as you when I try to run the jsdom tests though (npm run test)

I'm using MacOS

I'll open an issue in the wasm-bindgen repo

@chinedufn
Copy link
Owner Author

Opened an issue here -> rustwasm/wasm-bindgen#857

@chinedufn
Copy link
Owner Author

@qm3ster cargo install wasm-bindgen-cli --git https://github.com/rustwasm/wasm-bindgen --force should do the trick.

As explained in that issue it's because we're depending on git URLs for our wasm-bindgen deps. Which we'll eventually stop doing after the web-sys crate is published

@qm3ster
Copy link
Contributor

qm3ster commented Sep 19, 2018

Maybe we should install that binary locally into the project?
Dockerfile needs it too, then.

@chinedufn
Copy link
Owner Author

web-sys is scheduled to be published this month (unless things change) so I'm almost thinking that we can deal with the nuisance until it (hopefully) disappears soon

@qm3ster
Copy link
Contributor

qm3ster commented Sep 19, 2018

I'm on 🅱️u🅱️untu and recently I've been having nasty cases of unkillable cargo. RIP

@qm3ster
Copy link
Contributor

qm3ster commented Sep 19, 2018

Do you use Discord?

@chinedufn
Copy link
Owner Author

Haha I don't - should I be?

@qm3ster
Copy link
Contributor

qm3ster commented Sep 20, 2018

Just don't know where to talk more synchronously.
I am trying to rebase my branch on master and fix the docker build.
It seems that the git wasm-bindgen-cli fails on wasm compiled with --release :(

@chinedufn
Copy link
Owner Author

Ahh word -> I'm not sure what the best platforms are for chatting around open source projects but if ya wanna make one for percy i'd definitely join it

Blehhhhhhh yeah all of this should be less of a mess once web-sys is published and we can ditch these git URLs. I can ask the wasm-bindgen folks about the eta on publishing web-sys.


But in the meantime:

Did ya run cargo update in the virtual-dom-rs folder? And also pull the latest wasm-bindgen-cli from git? It only fails in release but not in debug builds?

@qm3ster
Copy link
Contributor

qm3ster commented Sep 21, 2018

Yeah, just changing it to build without release makes it work. 3ed971a

This makes the container build and run locally successfully for me. The wasm payload is 5x bigger though :(

Do you maybe wanna make a repo where travis would be publishing this to your DockerHub?

FROM yasuyuky/rust-nightly-musl:latest as build

# Install wasm32-unknown-unknown-target
RUN rustup default nightly
RUN rustup target add wasm32-unknown-unknown \
  x86_64-unknown-linux-musl

# Node.js & npm
RUN curl -sL https://deb.nodesource.com/setup_10.x | bash -
RUN apt-get install -y nodejs 

RUN curl -sL https://github.com/WebAssembly/binaryen/releases/download/version_51/binaryen-version_51-x86-linux.tar.gz\
  | tar -xzf - binaryen-version_51/wasm-opt --strip-components=1 &&\
  chmod +x ./wasm-opt &&\
  mv ./wasm-opt /usr/local/bin/

# Install WASM bindgen CLI
# RUN curl -OL https://github.com/rustwasm/wasm-bindgen/releases/download/0.2.21/wasm-bindgen-0.2.21-x86_64-unknown-linux-musl.tar.gz &&\
#   tar xf wasm-bindgen-0.2.21-x86_64-unknown-linux-musl.tar.gz &&\
#   rm wasm-bindgen-0.2.21-x86_64-unknown-linux-musl.tar.gz &&\
#   chmod +x wasm-bindgen-0.2.21-x86_64-unknown-linux-musl/wasm-bindgen &&\
#   mv wasm-bindgen-0.2.21-x86_64-unknown-linux-musl/wasm-bindgen /usr/local/bin/wasm-bindgen
# TODO: revert this when `web-sys` is released to avoid compiling
RUN cargo install wasm-bindgen-cli --git https://github.com/rustwasm/wasm-bindgen

WORKDIR /usr/src

This could be used to speed up now.

@chinedufn
Copy link
Owner Author

Oh nice idea on having a base image with our deps

Would you like to own that? Maybe qm3ster/percy-dockerfile or something?

Or are you saying that it’s easier if I do it? Don’t mind either way!

@qm3ster
Copy link
Contributor

qm3ster commented Sep 21, 2018

Idk what the naming convention is. Locally I named it percy-build-docker.
I don't want to own anything, like what if I kms suddenly?
It think it would look like this:

services:
  - docker

before_install:
  - docker build -t LMAO_WHAT_ARE_TAGS_WHAT_GOES_HERE .

script:
  - echo "$DOCKER_PASSWORD" | docker login -u "$DOCKER_USERNAME" --password-stdin
  - docker push chinedufn/percy-builder

But idk about travis specifics. Eg, why they suggest before_install and not before_script, etc.

And you also need to do

travis env set DOCKER_USERNAME myusername
travis env set DOCKER_PASSWORD secretsecret

@chinedufn
Copy link
Owner Author

@qm3ster so WRT things being broken it looks like they're planning on publishing web-sys this week! rustwasm/wasm-bindgen#613 (comment)

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.

2 participants