Update tsconfigs & bundling

[lishaduck]

Depends on #158. Diff: https://github.com/lishaduck/sheriff/compare/bump-deps...updatetsconfigs

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: ac6f911cbcb113473033e73c51b9e1f7dbb10a3d

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

This PR includes changesets to release 8 packages

| Name | Type | | --------------------- | ----- | | create-sheriff-config | Patch | | eslint-config-sheriff | Patch | | @sherifforg/constants | Patch | | sheriff-webservices | Patch | | @sherifforg/types | Patch | | @sheriff/utils | Patch | | docs-website | Patch | | tsconfig | 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[bot]]

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	Jul 22, 2024 2:21am

[lishaduck]

Oh, just realize that this'll break #150 again. Shouldn't be hard fix though.

[lishaduck]

Took a look, we'll just need to add "noExternal": ["@sherifforg/constants"] to all of the tsup.config.json files. Speaking of which, they all need the same config. Do we just want to switch to .ts files, write one, and then reexport it?

[lishaduck]

I realized I had a question about this: do we want sourcemaps? It'd be even smaller to get rid of them, and I don't see how they'd help debug for an eslint config, but I'll leave that up to you.

[AndreaPontrandolfo]

@lishaduck yeah, i don't think we want to export sourcemaps, the end user doesn't need them.

[AndreaPontrandolfo]

Took a look, we'll just need to add "noExternal": ["@sherifforg/constants"] to all of the tsup.config.json files. Speaking of which, they all need the same config. Do we just want to switch to .ts files, write one, and then reexport it?

Do you happen to know the difference between using the noExternal flag and just passing the dep as a devdep, like i did in https://github.com/AndreaPontrandolfo/sheriff/pull/151? Pros/cons?

[lishaduck]

Took a look, we'll just need to add "noExternal": ["@sherifforg/constants"] to all of the tsup.config.json files. Speaking of which, they all need the same config. Do we just want to switch to .ts files, write one, and then reexport it?

Do you happen to know the difference between using the noExternal flag and just passing the dep as a devdep, like i did in #151? Pros/cons?

So, it was all bundled before. The dependencies fields made zero difference. Making it a devdep made it so that it didn't try to install, but it was still there. This PR makes it so that you only bundle the library, not the dependencies. As @sherifforg/constants isn't there, it would succeed to install, but fail at runtime. noExternal makes it bundle despite being in node_modules.

[AndreaPontrandolfo]

As @sherifforg/constants isn't there, it would succeed to install, but fail at runtime

I don't think this is correct. Adding a dep as devDependency makes TSUP bundle it inline. I was looking into the generated dist and it was in there, which is why i marked the bug as resolved.

By default tsup bundles all import-ed modules but dependencies and peerDependencies in your package.json are always excluded

This is the reason.

Or did i misunderstood something?

[lishaduck]

As @sherifforg/constants isn't there, it would succeed to install, but fail at runtime

I don't think this is correct. Adding a dep as devDependency makes TSUP bundle it inline. I was looking into the generated dist and it was in there, which is why i marked the bug as resolved.

Yeah, but it would've been there either way. The issue was with npm.

By default tsup bundles all import-ed modules but dependencies and peerDependencies in your package.json are always excluded

This is the reason.

Or did i misunderstood something?

Huh. That wasn't what I was seeing. It looked like it was bundling all of it. Either I misunderstood it, or it was buggy. Probably the former 🤷‍♂️ 😬 Either way, this is the "correct" way to do it.

[lishaduck]

Ok, I looked at the source, it seems like the CLI defaults to not bundling node_modules, but the API does, and the default in the presence of a config is (maybe) to bundle them? I didn't look super closely.

[AndreaPontrandolfo]

@lishaduck also this needs to be updated and conflicts resolved.

[lishaduck]

@lishaduck also this needs to be updated and conflicts resolved.

I'll get to it next week 🤞[^1]

[^1]: Praying that the school IT department didn't botch anything up and I can still use git. I can't sometimes for some reason.

[lishaduck]

"skipNodeModulesBundle": true, // You can try changing this, but I found bundling to bundle `node_modules` too, which isn't what we want
"minify": true, // I can remove this
"replaceNodeEnv": true, // On second thought, this really isn't needed, should remove
"treeshake": "recommended", // Again, can be removed
"removeNodeProtocol": false // Added in Tsup 8.2.0, will be the default in 9, so opting in to minimize the effect

[AndreaPontrandolfo]

"skipNodeModulesBundle": true, // You can try changing this, but I found bundling to bundle `node_modules` too, which isn't what we want
"minify": true, // I can remove this
"replaceNodeEnv": true, // On second thought, this really isn't needed, should remove
"treeshake": "recommended", // Again, can be removed
"removeNodeProtocol": false // Added in Tsup 8.2.0, will be the default in 9, so opting in to minimize the effect

skipNodeModulesBundle and removeNodeProtocol aren't documented anywhere and i really don't feel comfortable using undocumented behaviors unless there is a critical need (which is not our case now). Considering that TUSP is a VERY popular package in the ecosystem, I'm sure the default config options are very well thought-out and suitable for the vast majority of scenarios/projects. We don't need to bang our heads over this stuff too much :)

[lishaduck]

skipNodeModulesBundle and removeNodeProtocol aren't documented anywhere and i really don't feel comfortable using undocumented behaviors unless there is a critical need.

• removeNodeProtocol, fair enough.
• skipNodeModulesBundle, I'm going to check it again, but I still think it wasn't working without it (P.S. docs: https://tsup.egoist.dev/#excluding-all-packages, I believe tsup-node adds skipNodeModulesBundle).

[lishaduck]

I also discovered that the CJS build is broken by default, we need to enable cjsInterop (which requires splitting) 😢. Otherwise, it emits the wrong CJS types.

See: egoist/tsup#572, egoist/tsup#992, evanw/esbuild#3281.

[AndreaPontrandolfo]

skipNodeModulesBundle and removeNodeProtocol aren't documented anywhere and i really don't feel comfortable using undocumented behaviors unless there is a critical need.

• removeNodeProtocol, fair enough.
• skipNodeModulesBundle, I'm going to check it again, but I still think it wasn't working without it (P.S. docs: https://tsup.egoist.dev/#excluding-all-packages, I believe tsup-node adds skipNodeModulesBundle).

But in the docs it says:

tsup automatically excludes packages specified in the dependencies and peerDependencies fields in the package.json

Also:

I also discovered that the CJS build is broken by default

Can you be more specific on how it's broken? Any stacktrace?

[lishaduck]

But in the docs it says:

tsup automatically excludes packages specified in the dependencies and peerDependencies fields in the package.json

Correct. I think it's a bug. I'll look into it sometime when I have some time.

I also discovered that the CJS build is broken by default

Can you be more specific on how it's broken? Any stacktrace?

I haven't used the CJS build, so I don't have a stacktrace, but I recall noticing that it emits the export keyword instead of module.exports =. It's apparently an ESBuild limitation. cjsInterop is supposed to fix it, but it's broken without "splitting": true.

[AndreaPontrandolfo]

I haven't used the CJS build, so I don't have a stacktrace, but I recall noticing that it emits the export keyword instead of module.exports =. It's apparently an ESBuild limitation. cjsInterop is supposed to fix it, but it's broken without "splitting": true.

I just built eslint-config-sheriff locally (from master). It produces a index.js, a index.d.ts and a index.cjs. At the end of the index.cjs there is this:

// src/index.ts
var src_default = getExportableConfig;
// Annotate the CommonJS export names for ESM import in node:
0 && (module.exports = {
  allJsExtensions,
  allJsxExtensions,
  baseNoRestrictedSyntaxRules,
  ignores,
  sheriff,
  sheriffStartingOptions,
  supportedFileTypes,
  testsFilePatterns
});

I'm no master of Commonjs syntax but i think module.exports is alright, i guess?

[lishaduck]

I just built eslint-config-sheriff locally (from master). It produces a index.js, a index.d.ts and a index.cjs.

It's missing an index.d.cts, which is fixed on this branch except that is still uses export rather than module.exports. cjsInterop works around some other esbuild limitations too, but it should fix the types issue specifically.

[AndreaPontrandolfo]

i guess this is pretty much this issue. I guess we could try to meet all the requirements of @arethetypeswrong/cli in this PR.

[AndreaPontrandolfo]

So then we can add the are-the-types-wrong command in the merge-checks.

[lishaduck]

That was the goal, to get attwpublint passing. Currently, (almost) all sheriff packages are masquerading as ESM, this separates the files, but ESBuild currently still outputs ESM syntax in CJS .d.ts files, or something 🤷‍♂️. cjsInterop works around that by fixing in a post-processing step.

[lishaduck]

Just if you're curious, this is a good read: https://blog.isquaredsoftware.com/2023/08/esm-modernization-lessons/

I just refound it, and it was quite useful. P.S: I'm still working on this :)

[AndreaPontrandolfo]

Just if you're curious, this is a good read: https://blog.isquaredsoftware.com/2023/08/esm-modernization-lessons/

Oh yeah, i remember this, read this last year. Bu thanks!

P.S: I'm still working on this :)

I'm really looking forward to merge all your open PRs, sorry for the conflicts >_< I'm especially looking forward the "turbo watch" enablement.

[lishaduck]

I'm really looking forward to merge all your open PRs, sorry for the conflicts >_<

It's fine :)

I'm especially looking forward the "turbo watch" enablement.

I think that already got merged in #158

[AndreaPontrandolfo]

I'm especially looking forward the "turbo watch" enablement.

I think that already got merged in #158

We updated the dependency but didn't actually added the watch script to "listen" to tsup build tasks. I think i'm gonna add a proper issue for this.

[AndreaPontrandolfo]

Here we go https://github.com/AndreaPontrandolfo/sheriff/issues/227