muxinc / media-chrome

Custom elements (web components) for making audio and video player controls that look great in your website or app.
https://media-chrome.org
MIT License
1.11k stars 56 forks source link

task/ts-conversion #914

Closed littlespex closed 5 days ago

littlespex commented 1 month ago

This is an experiment in using Copilot and other tools to convert the code base to typescript. Remaining tasks:

vercel[bot] commented 1 month ago

@littlespex is attempting to deploy a commit to the Mux Team on Vercel.

A member of the Team first needs to authorize it.

dylanjha commented 1 month ago

@littlespex amazing stuff!

First pass I'm noticing a lot of linting-ish diffs. Spacing, quotes, etc. Stuff like that. We do have a yarn lint command and I do see in the commit messages either you or copilot workspace was making lint commits.

Did our existing eslint config not get applied? Or was the eslint config modified as part of this change?

littlespex commented 1 month ago

Those are the result of running the format script. The code generated by the tools had some sloppy formatting, so I ran npm run format to normalize it. Is that script still valid?

littlespex commented 1 month ago

@dylanjha @heff Does anyone know how to address this Playwright error: https://github.com/muxinc/media-chrome/actions/runs/9289849038/job/25566542757#step:10:23

The test --all command works for me locally, but something with the install of playwright is preventing CI from completing. I did update the package.json and yarn.lock, so I want to make sure these errors weren't introduced by this PR.

cjpillsbury commented 1 month ago

@dylanjha @heff Does anyone know how to address this Playwright error: https://github.com/muxinc/media-chrome/actions/runs/9289849038/job/25566542757#step:10:23

The test --all command works for me locally, but something with the install of playwright is preventing CI from completing. I did update the package.json and yarn.lock, so I want to make sure these errors weren't introduced by this PR.

I'm re-running now just in case. I can pull your branch down and test locally as well.

cjpillsbury commented 1 month ago

@littlespex looking like the buckshot updates did cause some dependency issues with the test runners. Will require some digging.

cjpillsbury commented 1 month ago

@littlespex looks like it's a yarn + playwright binaries issue. https://github.com/microsoft/playwright/issues/5767 Since we had some dependencies still pointing to npmjs registry that were updated to use the yarn registry, I'm wondering if that is the root cause. (We can also go down the path of doing the env setup workarounds if that makes sense)

dylanjha commented 1 month ago

Those are the result of running the format script. The code generated by the tools had some sloppy formatting, so I ran npm run format to normalize it. Is that script still valid?

@littlespex yeah, the format script should still be valid. But something seems off because it runs prettier and our prettier config specifies single quotes: https://github.com/muxinc/media-chrome/blob/692be61a2a63ba98c7e449d860ce1828b78353a2/package.json#L106-L110

But for some reason this diff is showing a lot of conversions from single quotes to double quotes. That's just one example, but the broader point is that mis-matched linting & formatting is generating a larger diff than necessary.

littlespex commented 1 month ago

Re-ran prettier and the quotes are fixed. There are other formatting changes, like print width, that were also applied using prettier's default values.

cjpillsbury commented 1 month ago

@littlespex I'm going to review the particular commits and see if we can identify any linting/formatting/prettier crud into a separate PR to make this easier.