Skip to content

Conversation

@lishaduck
Copy link
Contributor

Related Issue

Closes #388.

Other

@changeset-bot
Copy link

changeset-bot bot commented Apr 20, 2025

⚠️ No Changeset found

Latest commit: 1ee7d45

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Apr 20, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sheriff ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 28, 2025 2:10am

package.json Outdated
"typesync": "typesync --dry=fail",
"typecheck": "turbo run typecheck",
"knip": "knip",
"knip": "knip --treat-config-hints-as-errors",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just slipping this in (I requested this in webpro-nl/knip#891).

Choose a reason for hiding this comment

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

i would have preferred this as a config option (in the config file, i mean), but it's fine either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, see webpro-nl/knip#1026.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, and then he added it :)

Choose a reason for hiding this comment

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

we will wait for him to add this as a config option in the next release. I prefer it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I filed #397, I'll revert this from this PR.

"knip": "knip",
"knip": "knip --treat-config-hints-as-errors",
"validate-config": "turbo run validate-config",
"merge-checks": "pnpm build && turbo run publint manypkg typesync knip typecheck lint validate-config are-the-types-wrong && pnpm check-deduped-deps",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yay, this hack is done!

Choose a reason for hiding this comment

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

this wasn't a hack. This was here to check if the build was passing before doing the lints and the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Build is called as part of merge-checks (except for docs), putting it before is antithetical to turbo (i.e., the tasks are misconfigured), and slower.

If you'd like it reverted, I will, but I don't see why this would be the desired behavior.

"knip": "knip",
"knip": "knip --treat-config-hints-as-errors",
"validate-config": "turbo run validate-config",
"merge-checks": "pnpm build && turbo run publint manypkg typesync knip typecheck lint validate-config are-the-types-wrong && pnpm check-deduped-deps",

Choose a reason for hiding this comment

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

this wasn't a hack. This was here to check if the build was passing before doing the lints and the tests.

package.json Outdated
"typesync": "typesync --dry=fail",
"typecheck": "turbo run typecheck",
"knip": "knip",
"knip": "knip --treat-config-hints-as-errors",

Choose a reason for hiding this comment

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

we will wait for him to add this as a config option in the next release. I prefer it.

@lishaduck lishaduck force-pushed the faster-simpler-bundles-2 branch from 1ee7d45 to 5217d0f Compare April 28, 2025 02:10
- esbuild
- rolldown
- sharp
- unrs-resolver

Choose a reason for hiding this comment

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

missing: "@tailwindcss/oxide"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not rebased on main, oxide isn't needed (yet)?

"composite": false,
"target": "ESNext",
"module": "Preserve",
"module": "ESNext",

Choose a reason for hiding this comment

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

this needs to be removed

platform: 'node',
clean: true,
sourcemap: false,
skipNodeModulesBundle: true,

Choose a reason for hiding this comment

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

We can remove this.

entry: ['./src/index.ts'],
format: 'esm',
target: 'node20',
platform: 'node',

Choose a reason for hiding this comment

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

This is the default. We can remove this.

{
"$schema": "https://json.schemastore.org/tsconfig",
"include": ["src", "eslint.config.ts"],
"exclude": ["dist", "build", "node_modules"],
Copy link
Owner

@AndreaPontrandolfo AndreaPontrandolfo Jun 12, 2025

Choose a reason for hiding this comment

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

im not entirely sure why you removed the "excludes", but we want to keep them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're already inherited from the base tsconfig, I think?

import { defineConfig } from 'tsdown';

export default defineConfig({
entry: ['./src/index.ts'],

Choose a reason for hiding this comment

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

This is the default. We can remove this.

"typecheck": "tsc",
"lint": "eslint ./src --max-warnings=0",
"publint": "publint"
"prepare": "pnpm build"

Choose a reason for hiding this comment

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

what's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the missing bin warning.

},
},
});
{ ignores: ['tsdown.config.ts'] },

Choose a reason for hiding this comment

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

This file doesn't need to be modified at all.

Comment on lines +38 to +50
export const allJsExtensions: string = `${jsExtensions},${tsExtensions}`;

export const allJsxExtensions = `${jsxExtensions},${tsxExtensions}`;
export const allJsxExtensions: string = `${jsxExtensions},${tsxExtensions}`;

export const supportedFileTypes = `**/*{${allJsExtensions},${allJsxExtensions},astro}`;
export const supportedFileTypes: string = `**/*{${allJsExtensions},${allJsxExtensions},astro}`;

export const testsFilePatterns = [
export const testsFilePatterns: string[] = [
`**/*.{test,spec}.{${allJsExtensions}}`,
`**/tests/**/*.{${allJsExtensions}}`,
`**/__tests__/**/*.{${allJsExtensions}}`,
] as const;

export const ignores = [
export const ignores: string[] = [

Choose a reason for hiding this comment

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

unneded changes.

@lishaduck
Copy link
Contributor Author

lishaduck commented Jun 22, 2025

I'm no longer actively using Sheriff (Writing deputy1), and am busy IRL anyway. I'd be happy to finish this up someday, but I doubt I'll find the time anytime soon, so feel free to do it yourself or just stick with tsup.

Footnotes

  1. Code at https://github.com/lishaduck/deputy/tree/wip. I essentially forked Sheriff to fix [ADD] eslint-plugin-svelte #337 & Split it up/optional dependencies/etc #257, with some other more opinionated changes. Currently blocked on some internal ESLint type fixes.

@lishaduck lishaduck closed this Jun 22, 2025
@AndreaPontrandolfo
Copy link
Owner

I'm no longer actively using Sheriff (Writing deputy1), and am busy IRL anyway. I'd be happy to finish this up someday, but I doubt I'll find the time anytime soon, so feel free to do it yourself or just stick with tsup.

Footnotes

  1. Code at https://github.com/lishaduck/deputy/tree/wip. I essentially forked Sheriff to fix [ADD] eslint-plugin-svelte #337 & Split it up/optional dependencies/etc #257, with some other more opinionated changes. Currently blocked on some internal ESLint type fixes.

Alrighty, got it 👍

@AndreaPontrandolfo
Copy link
Owner

  1. Code at https://github.com/lishaduck/deputy/tree/wip

@lishaduck Im excited so see how your solution for optional deps turns out! Its tricky stuff for sure!

@lishaduck
Copy link
Contributor Author

lishaduck commented Jun 23, 2025

  1. Code at https://github.com/lishaduck/deputy/tree/wip

@lishaduck Im excited so see how your solution for optional deps turns out! Its tricky stuff for sure!

Thanks! It was working pretty well except for some stuff with typechecked rule configs, which didn't like the monorepo. There's some momentum there, so 🤞 I can get back to work on it soon.
Oh, and it handles #61 as well (EDIT: and #203).

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.

[INTERNAL] Migrate to tsdown

2 participants