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

build: use vite for live-reload mode in @penrose/panels and @penrose/browser-ui #929

Merged
merged 9 commits into from
Apr 26, 2022

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented Apr 18, 2022

Description

Resolves #913; see also #552

As reported in #913, serve-http causes errors on Linux because it uses Node's watch function. We use serve-http for the estrella live-watch mode and unfortunately it's not actively maintained. Since we've already transitioned synthesizer-ui to vite, it makes more sense for us to use it for panels and browser-ui.

Implementation strategy and design decisions

  • Add vite configuration to both packages
  • Tested live-watch mode on OSX

Examples with steps to reproduce them

  • yarn start for browser-ui
  • yarn start:ide for panels

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated changes to the diagrams/ folder

Open questions

  • Still need to test on linux. @samestep can you try whenever you have a chance?
  • I'm keeping estrella for building both packages, because I couldn't get vite build to work :(.

@cloudflare-pages
Copy link

cloudflare-pages bot commented Apr 18, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 11ca133
Status: ✅  Deploy successful!
Preview URL: https://2225ee4a.penrose-panes.pages.dev

View logs

@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #929 (ba72793) into main (9513462) will increase coverage by 1.51%.
The diff coverage is 67.14%.

❗ Current head ba72793 differs from pull request most recent head 11ca133. Consider uploading reports for the commit 11ca133 to get more accurate results

@@            Coverage Diff             @@
##             main     #929      +/-   ##
==========================================
+ Coverage   68.15%   69.66%   +1.51%     
==========================================
  Files          62       62              
  Lines        8123     8371     +248     
  Branches     1771     1828      +57     
==========================================
+ Hits         5536     5832     +296     
+ Misses       2580     2532      -48     
  Partials        7        7              
Impacted Files Coverage Δ
packages/core/src/synthesis/Synthesizer.ts 28.13% <33.74%> (+8.93%) ⬆️
packages/core/src/synthesis/Search.ts 82.14% <67.94%> (-5.96%) ⬇️
packages/core/src/analysis/SubstanceAnalysis.ts 75.82% <82.60%> (+26.13%) ⬆️
packages/core/src/synthesis/Mutation.ts 50.48% <83.06%> (+31.80%) ⬆️
packages/core/src/compiler/Style.ts 86.26% <85.71%> (+<0.01%) ⬆️
packages/core/src/compiler/Substance.ts 92.36% <98.86%> (+1.43%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9513462...11ca133. Read the comment docs.

@wodeni wodeni changed the title build: use vite for live-watch mode in @penrose/panels and @penrose/browser-ui build: use vite for live-reload mode in @penrose/panels and @penrose/browser-ui Apr 19, 2022
@samestep
Copy link
Collaborator

Still need to test on linux. @samestep can you try whenever you have a chance?

Sure, I'll see if I can do that this evening.

@wodeni
Copy link
Member Author

wodeni commented Apr 21, 2022

Update: yarn start works for browser-ui, but yarn start:ide actually doesn't work because of an error in octokit. Kinda surprised that we had it working in the first place.

Related issue: vitejs/vite#5963

I tried using octokit instead of @octokit/rest. Didn't seem to help.

@samestep
Copy link
Collaborator

Tested on Linux just now and this does indeed fix the yarn start issue.

@elviejo79
Copy link

This fix didn't work for me on: Ubuntu 20.04.

With yarn and node versions:

agarciafdz@agarciafd-lg:~/r/gh/penrose/penrose$ yarn --version
1.22.15
agarciafdz@agarciafd-lg:~/r/gh/penrose/penrose$ node --version
v17.4.0

The error:

@penrose/browser-ui: ✘ [ERROR] [plugin vite:dep-scan] Failed to resolve entry for package "@penrose/core". The package may have incorrect main/module/exports specified in its package.json: Failed to resolve entry for package "@penrose/core". The package may have incorrect main/module/exports specified in its package.json.

Complete installaition log is attached.

yarn_install.log

@samestep
Copy link
Collaborator

@elviejo79 Thanks for pointing this out! I was able to reproduce. This happens if you run yarn, then yarn start, from a clean clone. If you run yarn build before running yarn start then that error does not occur.

@wodeni It seems bad that yarn build is required before yarn start; is there a way to either remove this prerequisite step or at least make it less confusing so that people can know what to do if they try to run yarn start without yarn build?

@wodeni
Copy link
Member Author

wodeni commented Apr 25, 2022

@wodeni It seems bad that yarn build is required before yarn start; is there a way to either remove this prerequisite step or at least make it less confusing so that people can know what to do if they try to run yarn start without yarn build?

Yep, can confirm. I added a prestart script that builds core. The build is reasonably fast at this point (3s on my machine). Not the best solution, but lmk if you think it's a good idea @samestep.

Also, thanks @elviejo79 for helping!

@wodeni
Copy link
Member Author

wodeni commented Apr 26, 2022

Update: yarn start works for browser-ui, but yarn start:ide actually doesn't work because of an error in octokit. Kinda surprised that we had it working in the first place.

Related issue: vitejs/vite#5963

I tried using octokit instead of @octokit/rest. Didn't seem to help.

Fixed by adding node polyfills in vite.config.js in 11ca133

@wodeni wodeni merged commit 4d41b0c into main Apr 26, 2022
@wodeni wodeni deleted the fix-watch branch April 26, 2022 20:18
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.

Does not work on Linux due to "recursive watch"
3 participants