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

Add example using vite #173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nmrshll
Copy link
Contributor

@nmrshll nmrshll commented Jan 8, 2024

closes #172

@StuartHarris
Copy link
Member

Thank you @nmrshll for contributing this. We'd be happy to merge it once a few things are sorted.

  • it looks to me like this belongs to the simple_counter example rather than the counter example, so it should be moved
  • The view model property to display the count should therefore be .count rather than .text — e.g. <button>{view.getView().count}</button>
  • I couldn't see where the shared library is being built (maybe I'm missing something). Maybe the package.json scripts should look something like this?
  "scripts": {
    "dev": "pnpm run wasm:build && vite",
    "build": "pnpm run wasm:build && tsc && vite build",
    "preview": "vite preview",
    "wasm:build": "cd ../shared && wasm-pack build --target web"
  },

@@ -0,0 +1,13 @@
<!doctype html>
Copy link
Member

Choose a reason for hiding this comment

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

I think all this belongs in simple_counter, rather than counter

"private": true,
"version": "0.0.0",
"type": "module",
"scripts": {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to build the shared lib?

  "scripts": {
    "dev": "pnpm run wasm:build && vite",
    "build": "pnpm run wasm:build && tsc && vite build",
    "preview": "vite preview",
    "wasm:build": "cd ../shared && wasm-pack build --target web"
  }

<>
<h1>Vite + Solid</h1>
<div class="card">
<button>count is {view.getView().text}</button>
Copy link
Member

Choose a reason for hiding this comment

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

<button>{view.getView().count}</button>

@StuartHarris
Copy link
Member

When I tested it, there was no styling. Any chance you could bring that in too? Thank you!

@nmrshll
Copy link
Contributor Author

nmrshll commented Feb 2, 2024

Thanks for the review !
Sure thing, I'll make those few changes.
(I think I entirely missed that there were two different counter examples and mixed them up)

@nmrshll nmrshll force-pushed the 172-add-example-using-vite branch from 9a0821b to 811bb4c Compare February 13, 2024 13:40
@nmrshll nmrshll force-pushed the 172-add-example-using-vite branch from 811bb4c to 1551f58 Compare February 13, 2024 13:50
@nmrshll
Copy link
Contributor Author

nmrshll commented Feb 13, 2024

I've done those changes you needed:

  • move to example simple_counter
  • make package.json scripts build the shared wasm library
  • bring back the styling I had stripped before (btw, first time I use bulma, but it seems great for keeping small examples simple)

Let me know if it looks better now (whenever you've got the time, no rush)

@StuartHarris
Copy link
Member

Hey @nmrshll this looks great now! Thank you for contributing. There's just a couple of minor things. The first is that we'll need a .gitignore file to exclude node_modules.

The second is that the update() function initialises the Wasm module every time it is called. It looks like this is idempotent, and so probably safe, but it does mean that there could be a slight delay (especially on poor bandwidth connections) when a button is first pressed, while the wasm is downloaded and initialised. I think a better approach would be to initialise the wasm module in index.tsx, instead. It looks like top-level await is supported so this should work...

import initCore from "shared";
await initCore();

Other than that would be happy to merge. Thank you.

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.

Add an example using vite as a JS/TS build tool
2 participants