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

Bump deps to latest major version and other misc dev UX #146

Merged
merged 36 commits into from
Apr 8, 2024

Conversation

fire332
Copy link
Collaborator

@fire332 fire332 commented Mar 29, 2024

Changes

  • Bump all dependencies to their latest major version. I have reviewed the changes for all the dependencies and they either don't break anything we do or I've fixed it. Things to note:
  • Added more ESLint rules that weren't enabled by the recommended config. No linting errors were introduced.
  • Cleaned up tooling configs by removing options that effectively do nothing.
  • Fix deploy script broken by CI, dependency, and metadata updates #128.
  • Tooling configs are now type checked and linted.
  • Removed deprecated VSCode extension from recommendation.
  • Switched to ESLint flat config.
  • Add type-check script and added all required @types to devDependencies
  • Added "type": "module to package.json. This means .js files ran by Node will be interpreted as ESM by default.
  • lint-staged now runs ESLint and tsc as well.

tools/deploy.mjs Outdated Show resolved Hide resolved
@throwaway96
Copy link
Member

Just a couple of tangentially-related notes:

I would like to move to the ESLint flat config format before our configs get too complex. Since it's currently mostly just rules, porting shouldn't be too bad. But I want to use eslint-plugin-prettier, and once you start with plugins it can get messy.

Does ares-install work reliably for you? I've had to come up a custom deploy script, since ares-install fails like 80% of the time for me (and I'm not the only one).

The lack of trailing commas drives me a bit nuts (merge conflicts...), but I'm not going to change it at least until there are fewer pending PRs.

@fire332
Copy link
Collaborator Author

fire332 commented Mar 29, 2024

Does everything else work on Windows?

I normally run npm run build && npm run package && npm run deploy every time I want to view my changed so it better work 😅.

Does ares-install work reliably for you?

Yes, I've went through 3 versions of ares-cli and it's my main method of installing.

I would like to move to the ESLint flat config format before our configs get too complex.

I don't have any experience with flat configs because it was only a few weeks ago that typescript-eslint started supporting flat configs and I had been busy since that update came out 😕.

@fire332
Copy link
Collaborator Author

fire332 commented Mar 29, 2024

I want to use eslint-plugin-prettier

Any particular reason why? I find that having squigglies in the editor for formatting issues just get in the way so I usually just setup my dev workflows to have auto formatting be as behind-the-scenes as possible.

In addition, mixing prettier issues with ESLint means that if you ever want to make automated PR code reviews from ESLint running in GH workflows, prettier errors will bleed in and spew dozens of garbage reviews when a simple "this PR needs to be formatted with prettier" status check would suffice.

@throwaway96
Copy link
Member

I like to see what changes formatters are making, including so that I'm less likely to be surprised by them mangling code. The ESLint plugin seems to be the recommended way to accomplish that.

In my experience the output for automated checks on PRs isn't that bad, but I suppose it could be worked around if it becomes an issue. We have automatic linting of PRs with eslint-plugin-prettier on Homebrew Channel, and it seems to work pretty well.

@throwaway96 throwaway96 added this to the v0.3.4 milestone Mar 30, 2024
@throwaway96 throwaway96 self-assigned this Mar 30, 2024
@fire332
Copy link
Collaborator Author

fire332 commented Apr 6, 2024

...

Quality GitHub UX design.

https://github.com/orgs/community/discussions/10369

@fire332
Copy link
Collaborator Author

fire332 commented Apr 6, 2024

Done. Do you want me configure lint-staged to run ESLint and tsc as well?

@throwaway96
Copy link
Member

Not in this PR. I'd like to get a new release done ASAP.

@fire332
Copy link
Collaborator Author

fire332 commented Apr 6, 2024

If time is the issue, I already have the config done for a previous project. I just need to paste it in.

@throwaway96
Copy link
Member

I guess if you can do it in like the next 30 minutes.

@fire332
Copy link
Collaborator Author

fire332 commented Apr 6, 2024

Done.

@fire332
Copy link
Collaborator Author

fire332 commented Apr 6, 2024

Oops. I just realized, I forgot to add lint-staged.config.js to tsconfig.tooling.json. Can you add and reformat the tsconfig? You should have permission to add commits to my repo.

@throwaway96
Copy link
Member

throwaway96 commented Apr 6, 2024

Just ran into this error while testing. No idea whether it's related to this PR.

VM22:2 Uncaught (in promise) TypeError: Kt is not a function
    at t.<anonymous> (<anonymous>:2:157100)
    at S (<anonymous>:2:110448)
    at Generator.<anonymous> (<anonymous>:2:111764)
    at Generator.next (<anonymous>:2:110851)
    at h (<anonymous>:2:116298)
    at a (<anonymous>:2:116489)
    at <anonymous>:2:116548
    at new Promise (<anonymous>)
    at new e (<anonymous>:2:31826)
    at t.<anonymous> (<anonymous>:2:116435)
    at t.<anonymous> (<anonymous>:2:157957)
    at <anonymous>:2:163908

It's on the line with return n = Kt(this.videoID).substring(0, 4),.

e = [{
key: "init",
value: (r = v(d().mark((function t() {
    var e, r, n, o, i, a, s, c = this;
    return d().wrap((function(t) {
        for (; ; )
            switch (t.prev = t.next) {
            case 0:
                return n = Kt(this.videoID).substring(0, 4),
                o = ["sponsor", "intro", "outro", "interaction", "selfpromo", "music_offtopic"],
                t.next = 4,
                fetch(Et(e = Et(r = "".concat("https://sponsorblock.inf.re/api", "/skipSegments/")).call(r, n, "?categories=")).call(e, encodeURIComponent(Nt(o))));

Looks like it corresponds to this:

const videoHash = sha256(this.videoID).substring(0, 4);

Is it possible to manually specify a source map for a userscript (it seems to always appear as VM22)? I haven't been able to figure out a way.

@fire332
Copy link
Collaborator Author

fire332 commented Apr 6, 2024

Is it possible to manually specify a source map for a userscript (it seems to always appear as VM22)? I haven't been able to figure out a way.

We can swap from devtool: 'source-map' to inline-source-map.

@throwaway96
Copy link
Member

We can swap from devtool: 'source-map' to inline-source-map.

Thanks, that does work. But I'm still not sure how tiny-sha256 broke.

@fire332
Copy link
Collaborator Author

fire332 commented Apr 6, 2024

Does webpack strip source maps from production bundles? Have you tested the dev bundle? Sorry, I'll be unavailable for the next half hour or so.

@throwaway96
Copy link
Member

Changing

import * as sha256 from 'tiny-sha256';

to

import sha256 from 'tiny-sha256';

fixes the issue. But I don't actually know JavaScript, so I have no idea why.

@fire332
Copy link
Collaborator Author

fire332 commented Apr 7, 2024

The former collects all exports into an object while the latter only uses the default export for ESM. For CommonJS interop, most bundlers will convert module.exports to the default export and module.exports.anything into a named export. But sometimes a package has a bad package.json or the bundler has some broken logic which makes "* as something" work.

@throwaway96
Copy link
Member

So some change to webpack/babel/whatever in this PR means the latter form is now required?

@fire332
Copy link
Collaborator Author

fire332 commented Apr 7, 2024

Yes. It's standard practice anyways. The former is a holdover from when ESM was new and bundler devs didn't really know how interop should work.

@throwaway96
Copy link
Member

I think it's working, but I can't really test it because the SponsorBlock server seems to be down.

@throwaway96 throwaway96 merged commit ffa5e32 into webosbrew:main Apr 8, 2024
1 check passed
throwaway96 added a commit that referenced this pull request Apr 8, 2024
@fire332 fire332 mentioned this pull request Apr 8, 2024
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