Are the types wrong?

[lishaduck]

Yes they were!

Fixed by dropping CJS from the builds following #225.

Supersedes/closes #161.

[changeset-bot]

🦋 Changeset detected

Latest commit: dad6ad9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
eslint-config-sheriff	Minor
@sherifforg/constants	Minor
@sherifforg/types	Minor
@sherifforg/cli	Minor
tsconfig	Minor
sheriff-webservices	Patch
docs-website	Patch

Not sure what this means? Click here to learn what changesets are.

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

[vercel]

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	Aug 25, 2024 9:09pm

[lishaduck]

CI is failing b/c it doesn't exclude as expected. I posted dist/index.js to a gist: https://gist.github.com/lishaduck/0a0d893fc6b3877b82aa29d17f4266f2 (search for node_modules/.pnpm/)

[lishaduck]

Oh, you know... that nice noExternal thing isn't going to keep #150 solved. NPM'll still try to install it. The published package.json needs to not include it all, not just in the bundle. It's in the bundle either way. I'm going to revert that change.

EDIT: Reverted

[AndreaPontrandolfo]

@lishaduck you know, i'm thinking an even better thing we could do to simplify stuff.
Just like with constants, we shouldn't ship the @sherifforg/types as a separate package. We can just re-export it from eslint-config-sheriff.
I don't see any downsides to it, only upsides, now that i think about it.

[lishaduck]

I don't see any downsides to it, only upsides, now that i think about it.

If I can get tsup to bundle constants, yeah, that'd work.
Right now, it's not bundling constants, which breaks a ton. I'm going to try both noExternal + devDep and see if that helps.

[lishaduck]

I don't see any downsides to it, only upsides, now that i think about it.

If I can get tsup to bundle constants, yeah, that'd work. Right now, it's not bundling constants, which breaks a ton. I'm going to try both noExternal + devDep and see if that helps.

That works! I also fixed the types (more undocumented behavior, sorry 😁)

[lishaduck]

Ok, downgraded my node and I can reproduce. I guess it's a node bug? Not blocking prod, just attw.

[lishaduck]

Ok, downgraded my node and I can reproduce. I guess it's a node bug? Not blocking prod, just attw.

Nope, it doesn't support the workspace protocol. Will need to switch to pnpm pack (arethetypeswrong/arethetypeswrong.github.io#173 (comment); note that the pr that fixed it was reverted)

[AndreaPontrandolfo]

Ok, downgraded my node and I can reproduce. I guess it's a node bug? Not blocking prod, just attw.

Nope, it doesn't support the workspace protocol. Will need to switch to pnpm pack (arethetypeswrong/arethetypeswrong.github.io#173 (comment); note that the pr that fixed it was reverted)

Ye, it seems like they deprecated the version with the PNPM support because it was breaking everything. Crazy!

[lishaduck]

Conflicts! Already!
I despair 😫

🤣

[lishaduck]

This should pass ci now and have 0 merge conflicts! Yay!
Only thought left is that maybe we should add a tsup-config internal package to keep it all in sync.

[lishaduck]

Also, it should be noted that despite attw claiming we support node 10, we don't 😅
I just suppressed the node10 errors, as they're not necessary for esm-only packages (they are for all cjs projects though, as node10 is the TS default for moduleResolution b/c history).

[lishaduck]

Forgot to push 🤦‍♂️

[AndreaPontrandolfo]

This should pass ci now and have 0 merge conflicts! Yay! Only thought left is that maybe we should add a tsup-config internal package to keep it all in sync.

Nah, it's fine. The eslint config and the CLI have different requirements. It makes sense that they rely on different bundling configs.

[AndreaPontrandolfo]

Also, it should be noted that despite attw claiming we support node 10, we don't 😅 I just suppressed the node10 errors, as they're not necessary for esm-only packages (they are for all cjs projects though, as node10 is the TS default for moduleResolution b/c history).

i guess we need to keep an eye on this: arethetypeswrong/arethetypeswrong.github.io#112

[AndreaPontrandolfo]

CI fails with:

Error: 1:30 error Parse errors in imported module 'eslint-config-sheriff': sourceType 'module' is not supported when ecmaVersion < 2015. Consider adding { ecmaVersion: 2015 } to the parser options. (undefined:undefined) import/no-named-as-default

I'm not exactly sure why this error surfaces here and now.
Anyway, to fix it, i think it's enough to add a ecmaVersion here:

      files: [supportedFileTypes],
      plugins: { import: fixupPluginRules(pluginImport) },
      rules: importHandPickedRules,
      languageOptions: {
        parserOptions: {
          sourceType: 'module', // required for https://github.com/import-js/eslint-plugin-import/blob/09476d7dac1ab36668283f9626f85e2223652b37/src/rules/no-default-export.js#L16
        },
      },

In this file: packages/eslint-config-sheriff/src/getBaseConfig.ts.
We can try { ecmaVersion: "latest" }, if that works. Otherwise we will have to specify a version, like 6, or 2022...

[lishaduck]

CI fails with:

Error: 1:30 error Parse errors in imported module 'eslint-config-sheriff': sourceType 'module' is not supported when ecmaVersion < 2015. Consider adding { ecmaVersion: 2015 } to the parser options. (undefined:undefined) import/no-named-as-default

I'm not exactly sure why this error surfaces here and now. Anyway, to fix it, i think it's enough to add a ecmaVersion here:

      files: [supportedFileTypes],
      plugins: { import: fixupPluginRules(pluginImport) },
      rules: importHandPickedRules,
      languageOptions: {
        parserOptions: {
          sourceType: 'module', // required for https://github.com/import-js/eslint-plugin-import/blob/09476d7dac1ab36668283f9626f85e2223652b37/src/rules/no-default-export.js#L16
        },
      },

In this file: packages/eslint-config-sheriff/src/getBaseConfig.ts. We can try { ecmaVersion: "latest" }, if that works. Otherwise we will have to specify a version, like 6, or 2022...

Yeah, I'd fixed it locally but, as always, forgot to push it 😅

[AndreaPontrandolfo]

@lishaduck can you add the changeset?

[lishaduck]

I'm excited to note that this drastically shrunk the size of dist/index.js, from 17,002 lines to 1,159.

EDIT: Though I realized that the actual types are still wrong 🤣 (see egoist/tsup#366)