Update tsconfigs & bundling #161

lishaduck · 2024-07-12T18:26:41Z

Depends on #158.
Diff: lishaduck/sheriff@bump-deps...updatetsconfigs

changeset-bot · 2024-07-12T18:26:44Z

🦋 Changeset detected

Latest commit: ac6f911

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 · 2024-07-12T18:26:45Z

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 · 2024-07-12T21:45:15Z

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

lishaduck · 2024-07-12T23:21:21Z

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 · 2024-07-14T20:50:58Z

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 · 2024-07-14T21:54:23Z

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

AndreaPontrandolfo · 2024-07-14T21:58:09Z

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?

lishaduck · 2024-07-14T22:20:54Z

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 · 2024-07-14T22:29:03Z

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 · 2024-07-15T01:42:41Z

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 · 2024-07-15T01:43:39Z

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 · 2024-08-01T17:57:34Z

packages/create-sheriff-config/package.json

-  "main": "dist/index.js",
+  "exports": {
+    ".": {
+      "node": "./dist/index.js"


i'm ok with "exports", it's better than "main", but i didn't know you could specify "node" here. I'll look up the docs.

Looked the docs.
It says:

"node" - matches for any Node.js environment. Can be a CommonJS or ES module file. In most cases explicitly calling out the Node.js platform is not necessary.

So maybe that's not necessary? Would be the same as default? Or not putting anything?

I think I assumed it acted like a bin, but I'm not sure why 🤷‍♂️
Yeah, default should work fine.

ya, we should refactor this then.

AndreaPontrandolfo · 2024-08-01T18:00:27Z

packages/tsconfig/base.json

  "compilerOptions": {
    "composite": false,
-    "target": "ESNext",
-    "module": "ESNext",
-    "moduleResolution": "bundler",


Why this? You wrote "moduleResolution": "bundler" in all other configs, why just not having it here instead?

I assumed that they were all different (not all of them are bundled), but everything else (i.e., Node16) breaks. I can recentralize it if desired.

AndreaPontrandolfo · 2024-08-01T18:01:56Z

packages/sheriff-types/tsup.config.json

mmm, what's the point of this?

We bundle sheriff-types so that the esm/cjs split is respected.

AndreaPontrandolfo · 2024-08-01T18:16:22Z

packages/sheriff-constants/tsup.config.json

treeshake and minify seems a bit overkill for a single 100-lines file with just a bunch of hard-coded strings in it. Also keeping it non-minified can be good for letting the consumer debugging easier (also raw strings don' get minifed anyway, so what's the point?)

In constants, yes, but I figured keeping them all consistent was convenient, and I didn't notice a difference in compile times.
I figured that for strings, minification wouldn't minify them, so it didn't make a major debugging hurdle.

AndreaPontrandolfo · 2024-08-01T18:20:56Z

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

lishaduck · 2024-08-01T18:57:45Z

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

I'll get to it next week 🤞¹

Praying that the school IT department didn't botch anything up and I can still use git. I can't sometimes for some reason. ↩

AndreaPontrandolfo · 2024-08-02T21:03:34Z

packages/create-sheriff-config/package.json

-  "main": "dist/index.js",
+  "exports": {
+    ".": {
+      "node": "./dist/index.js"


ya, we should refactor this then.

AndreaPontrandolfo · 2024-08-02T21:05:17Z

packages/eslint-config-sheriff/tsconfig.json

+    "lib": ["ES2021"],
+    "module": "ESNext",
+    "moduleResolution": "Bundler",
+    "allowImportingTsExtensions": true


what problem is this solving? Any specific type error that arised?

AndreaPontrandolfo · 2024-08-02T21:06:00Z

packages/eslint-config-sheriff/tsup.config.json

+  "clean": true,
+  "target": "node20",
+  "skipNodeModulesBundle": true,
+  "noExternal": ["@sherifforg/constants"],


This is great, but in tandem we should also revert the old fix that i did, meaning bringing back this dep to the regular deps instead of devdeps

AndreaPontrandolfo · 2024-08-02T21:06:47Z

packages/create-sheriff-config/tsup.config.json

+  "clean": true,
+  "target": "node20",
+  "skipNodeModulesBundle": true,
+  "noExternal": ["@sherifforg/constants"],


This is great, but in tandem we should also revert the old fix that i did, meaning bringing back this dep to the regular deps instead of devdeps

AndreaPontrandolfo · 2024-08-02T21:11:20Z

packages/tsconfig/base.json

  "compilerOptions": {
    "composite": false,
-    "target": "ESNext",
-    "module": "ESNext",
-    "moduleResolution": "bundler",


AndreaPontrandolfo

Can we also remove these from everywhere?

  "skipNodeModulesBundle": true,
  "minify": true,
  "replaceNodeEnv": true,
  "treeshake": "recommended",
  "removeNodeProtocol": false

Some of these apis aren't even documented anywhere, i have no clue what they do or what problem do they solve.
Treeshake: i don't think has any usecase here.
Minify: i would avoid because it risks of hiding the compiled code.

This stuff seems to be unnecessary and add complexity to the whole without adding any tangible value.

lishaduck · 2024-08-02T23:58:15Z

"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 · 2024-08-03T09:21:58Z

"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 · 2024-08-08T15:44:34Z

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 · 2024-08-08T15:45:11Z

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 · 2024-08-11T13:26:14Z

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 · 2024-08-11T19:45:10Z

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 · 2024-08-11T20:54:57Z

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 · 2024-08-11T20:58:42Z

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 · 2024-08-15T23:27:08Z

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 · 2024-08-15T23:28:16Z

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

lishaduck · 2024-08-16T00:41:34Z

That was the goal, to get ~~attw~~publint 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 · 2024-08-23T23:38:46Z

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 · 2024-08-23T23:52:46Z

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 · 2024-08-23T23:55:23Z

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 · 2024-08-24T00:00:50Z

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 · 2024-08-24T00:20:36Z

Here we go #227

vercel bot deployed to Preview July 12, 2024 18:27 View deployment

lishaduck force-pushed the updatetsconfigs branch from 9da39ee to a26c210 Compare July 12, 2024 19:26

vercel bot had a problem deploying to Preview July 12, 2024 19:26 Failure

lishaduck force-pushed the updatetsconfigs branch from a26c210 to 6e40547 Compare July 12, 2024 19:27

vercel bot had a problem deploying to Preview July 12, 2024 19:27 Failure

lishaduck force-pushed the updatetsconfigs branch from 6e40547 to de7bfbd Compare July 12, 2024 19:31

vercel bot had a problem deploying to Preview July 12, 2024 19:31 Failure

vercel bot deployed to Preview July 12, 2024 19:51 View deployment

lishaduck marked this pull request as draft July 12, 2024 21:45

lishaduck force-pushed the updatetsconfigs branch from d60d842 to 2ab1c49 Compare July 12, 2024 23:49

lishaduck marked this pull request as ready for review July 12, 2024 23:50

vercel bot deployed to Preview July 12, 2024 23:51 View deployment

This was referenced Jul 13, 2024

[ADD] eslint-plugin-n #163

Open

Don't require installing @typescript-eslint/parser #166

Closed

New blog post #167

Closed

lishaduck force-pushed the updatetsconfigs branch from 2ab1c49 to 357856e Compare July 16, 2024 15:12

vercel bot deployed to Preview July 16, 2024 15:15 View deployment

lishaduck added 2 commits July 21, 2024 18:45

chore(deps-dev): bump turbo

4f3b0ea

chore(deps-dev): bump

c06fc0b

AndreaPontrandolfo assigned lishaduck Jul 30, 2024

AndreaPontrandolfo self-requested a review July 30, 2024 21:11

AndreaPontrandolfo requested changes Aug 1, 2024

View reviewed changes

AndreaPontrandolfo mentioned this pull request Aug 1, 2024

Lint eslint-config-sheriff with eslint-config-sheriff #162

Closed

AndreaPontrandolfo requested changes Aug 2, 2024

View reviewed changes

lishaduck mentioned this pull request Aug 22, 2024

Remove CJS support for config file #222

Closed

This was referenced Aug 24, 2024

Are the types wrong? #229

Merged

Use configDir in workspace:tsconfig #235

Closed

AndreaPontrandolfo closed this in #229 Aug 25, 2024

lishaduck deleted the updatetsconfigs branch November 28, 2024 05:24

lishaduck mentioned this pull request Nov 28, 2024

Update Rules #169

Open

Update tsconfigs & bundling #161

Update tsconfigs & bundling #161

Conversation

lishaduck commented Jul 12, 2024

changeset-bot bot commented Jul 12, 2024 • edited Loading

🦋 Changeset detected

vercel bot commented Jul 12, 2024 • edited Loading

lishaduck commented Jul 12, 2024

lishaduck commented Jul 12, 2024

lishaduck commented Jul 14, 2024

AndreaPontrandolfo commented Jul 14, 2024

AndreaPontrandolfo commented Jul 14, 2024

lishaduck commented Jul 14, 2024

AndreaPontrandolfo commented Jul 14, 2024 • edited Loading

lishaduck commented Jul 15, 2024

lishaduck commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreaPontrandolfo commented Aug 1, 2024

lishaduck commented Aug 1, 2024

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreaPontrandolfo Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

AndreaPontrandolfo Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreaPontrandolfo left a comment • edited Loading

Choose a reason for hiding this comment

lishaduck commented Aug 2, 2024

AndreaPontrandolfo commented Aug 3, 2024

lishaduck commented Aug 8, 2024 • edited Loading

lishaduck commented Aug 8, 2024 • edited Loading

AndreaPontrandolfo commented Aug 11, 2024

lishaduck commented Aug 11, 2024 • edited Loading

AndreaPontrandolfo commented Aug 11, 2024

lishaduck commented Aug 11, 2024

AndreaPontrandolfo commented Aug 15, 2024

AndreaPontrandolfo commented Aug 15, 2024

lishaduck commented Aug 16, 2024 • edited Loading

lishaduck commented Aug 23, 2024

AndreaPontrandolfo commented Aug 23, 2024

lishaduck commented Aug 23, 2024

AndreaPontrandolfo commented Aug 24, 2024

AndreaPontrandolfo commented Aug 24, 2024

changeset-bot bot commented Jul 12, 2024 •

edited

Loading

vercel bot commented Jul 12, 2024 •

edited

Loading

AndreaPontrandolfo commented Jul 14, 2024 •

edited

Loading

AndreaPontrandolfo Aug 2, 2024 •

edited

Loading

AndreaPontrandolfo Aug 2, 2024 •

edited

Loading

AndreaPontrandolfo left a comment •

edited

Loading

lishaduck commented Aug 8, 2024 •

edited

Loading

lishaduck commented Aug 8, 2024 •

edited

Loading

lishaduck commented Aug 11, 2024 •

edited

Loading

lishaduck commented Aug 16, 2024 •

edited

Loading