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

fix: Resolve browser-ui build warnings #839

Merged
merged 9 commits into from Jan 14, 2022
Merged

Conversation

cmumatt
Copy link
Contributor

@cmumatt cmumatt commented Jan 14, 2022

Description

Closes issue #818

Resolves a number of typescript build errors in the browser-ui component: unsafe any types, unused variables, missing function return types, unsafe non-null assertions, etc.

Implementation strategy and design decisions

This is a cleanup activity to help developers (and CI) detect newly-introduced build errors and warnings.

Open questions

Similar issues in synthesizer-ui will be addressed next.

@cmumatt cmumatt self-assigned this Jan 14, 2022
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #839 (ca768e3) into main (6a89b03) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #839   +/-   ##
=======================================
  Coverage   69.08%   69.08%           
=======================================
  Files          62       62           
  Lines        7816     7816           
  Branches     1459     1459           
=======================================
  Hits         5400     5400           
  Misses       2403     2403           
  Partials       13       13           

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 6a89b03...ca768e3. Read the comment docs.

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jan 14, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: ca768e3
Status: ✅  Deploy successful!
Preview URL: https://6b82c504.penrose-panes.pages.dev

View logs

@cmumatt cmumatt marked this pull request as ready for review January 14, 2022 03:06
@cmumatt cmumatt requested a review from samestep January 14, 2022 03:06
@samestep
Copy link
Collaborator

samestep commented Jan 14, 2022

@cmumatt I see that you have a few commits re-running Prettier to get the format job to pass; I'm guessing that's due to VS Code producing different results from yarn format? @wodeni and I ran into similar issues in #836 (very annoying!), and we found that a solution is to be more explicit in our .prettierrc, to get VS Code to pick up on it; that's why that PR explicitly sets arrowParens and trailingComma to their default values.

On my machine I'm seeing similar issues in CompGraph.tsx:

const graphRef: React.RefObject<HTMLDivElement> = React.useRef<HTMLDivElement>(
null
);

VS Code tries to reformat those lines like this:

const graphRef: React.RefObject<HTMLDivElement> = React.useRef<
HTMLDivElement
>(null);

I'll see if I can figure out what Prettier option to explicitly set to resolve this discrepancy.

@samestep
Copy link
Collaborator

@cmumatt On a different note, I tried pushing a commit just now to see if getting rid of these build warnings would allow us to get rid this part of our CI config:

env:
CI: false # TODO: eliminate warnings so we don't have to do this

I tried pushing a commit to this branch removing those lines, but the build still seems to fail:

@penrose/browser-ui: Treating warnings as errors because process.env.CI = true.
@penrose/browser-ui: Most CI servers set it automatically.
@penrose/browser-ui: Failed to compile.
@penrose/browser-ui: ../core/build/dist/index.esm.js
@penrose/browser-ui: Critical dependency: the request of a dependency is an expression
@penrose/browser-ui: error Command failed with exit code 1.

Any idea if we can fix this? If not then we can just remove my commit (so, add that env setting back) and merge this anyways.

@samestep
Copy link
Collaborator

OK, after getting rid of Prettier 1.19.1 from the browser-ui devDependencies, VS Code still disagrees with yarn format but now in a different way:

  const graphRef: React.RefObject<HTMLDivElement> =
    React.useRef<HTMLDivElement>(null);

I'll continue investigating...

@samestep
Copy link
Collaborator

OK, let's ignore the Prettier stuff for now; I'm going to try to fix it by upgrading Prettier to the latest version, but it looks like that'll introduce a bunch of formatting changes to our codebase so I'll do it in a separate PR.

Copy link
Collaborator

@samestep samestep 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 to me! (modulo my earlier comment about env.CI)

@cmumatt
Copy link
Contributor Author

cmumatt commented Jan 14, 2022

@samestep sounds good. I will also break the post-ts->js warnings into a separate PR, similar to the build errors in synthesizer-ui. Thanks for looking into the prettier item. It took me a little bit to realize the IDE was formatting differently cli expects.

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

2 participants