solidjs / solid-start

SolidStart, the Solid app framework
https://start.solidjs.com
MIT License
5.04k stars 377 forks source link

[Feature?]: `@solidjs/start` should provide `.d.ts` #1228

Closed Brendonovich closed 6 months ago

Brendonovich commented 8 months ago

Duplicates

Latest version

Current behavior 😯

@solidjs/start currently points to .ts and .tsx files for resolving type information.

"types": "./shared/index.tsx",
"exports": {
  ".": "./shared/index.tsx",
  "./config": "./config/index.js",
  "./server": "./server/index.tsx",
  "./server/spa": "./server/spa/index.tsx",
  "./client": "./client/index.tsx",
  "./client/islands": "./client/islands.tsx",
  "./middleware": "./server/middleware.ts",
  "./client/spa": "./client/spa/index.tsx"
},
"typesVersions": {
  "*": {
    ".": [
      "./shared/index.tsx"
    ],
    "./server": [
      "./server/index.tsx"
    ],
    "./client": [
      "./client/index.tsx"
    ]
  }
},

Using source .ts files makes skipLibCheck ineffective, leading to errors like this

node_modules/.pnpm/@solidjs+start@0.4.4_rollup@2.79.1_solid-js@1.8.8_vinxi@0.1.1_vite@4.5.1/node_modules/@solidjs/start/shared/dev-overlay/CodeView.tsx:59:18 - error TS2532: Object is possibly 'undefined'.

59     const lang = props.fileName
                    ~~~~~~~~~~~~~~
60       .split(/[#?]/)[0]
   ~~~~~~~~~~~~~~~~~~~~~~~

node_modules/.pnpm/@solidjs+start@0.4.4_rollup@2.79.1_solid-js@1.8.8_vinxi@0.1.1_vite@4.5.1/node_modules/@solidjs/start/shared/dev-overlay/CodeView.tsx:80:11 - error TS18048: 'el' is possibly 'undefined'.

80           el.innerHTML = `<mark style="background-color:#aaaaaa80">${el.innerHTML}</mark>`;
             ~~

node_modules/.pnpm/@solidjs+start@0.4.4_rollup@2.79.1_solid-js@1.8.8_vinxi@0.1.1_vite@4.5.1/node_modules/@solidjs/start/shared/dev-overlay/CodeView.tsx:80:70 - error TS18048: 'el' is possibly 'undefined'.

80           el.innerHTML = `<mark style="background-color:#aaaaaa80">${el.innerHTML}</mark>`;
                                                                        ~~

node_modules/.pnpm/@solidjs+start@0.4.4_rollup@2.79.1_solid-js@1.8.8_vinxi@0.1.1_vite@4.5.1/node_modules/@solidjs/start/shared/dev-overlay/get-source-map.ts:14:20 - error TS2532: Object is possibly 'undefined'.

14     const result = lines[i].match(SOURCEMAP_REGEX);
                      ~~~~~~~~

Expected behavior πŸ€”

.d.ts files should be built and pointed to in types, exports and typesVersions in package.json. Source files should still be pointed to for conditional exports such as import and export. Prior art includes Solid Router and Kobalte.

Steps to reproduce πŸ•Ή

Steps:

  1. Open stackblitz
  2. Wait for packages to install
  3. Run pnpm tsc
  4. Observe a lot of errors from @solidjs/router even though skipLibCheck is enabled

Context πŸ”¦

I like doing monorepo-wide typechecking with pnpm tsc -b, but these errors get in the way. I can just run the command from packages that don't depend on @solidjs/start in the meantime.

Your environment 🌎

No response

ryansolid commented 8 months ago

Yeah we can look at adding a build process. This was a big problem for some projects in the previous version. But there is a lot less code here and while others working on this project might be all for JSDoc .. while I like the result, I generally am not.

There is some tension between the code needed for runtime and the code needed for build time. I need to make sure there isn't cross referencing in a weird way but this should be doable I think.

ryansolid commented 6 months ago

I was informed this would be problematic for things like the dev overlay. So I'm not sure how feasible this is at this time. I'm not the greatest with typescript and the other developers who contribute the most to the project have been pushing back from having a build process.

jakst commented 6 months ago

That's understandable, but I just want to emphasize how unconventional this setup is. I've worked in a lot of different code bases with tons of different frameworks, and never have I seen a package that ships like this, with type issues in the package itself affecting the end user. Most projects I have worked on enforce passing tsc in CI, meaning a solid-start project would not make it out the door. I do it myself for my hobby projects as well, and for solid-start I consider it okay because of the beta label, but if this is the intended end state for 1.0 as well I'm not sure I'll keep using solid-start. And please read this the right way, I absolutely love solid-start, but this kills my whole workflow which relies a lot on running tsc -w continuously while developing.

I don't have a lot of experience with publishing packages, but these are some ways forward that could alleviate this issue that I can come up with:

Just my 5 cents as a solid-js user since 3 years who really wants solid-start to succeed! πŸ™‚

Brendonovich commented 6 months ago

I think i've managed to add a simple build process using tsc and some file copying. It's available on npm under @brendonovich/solid-start and the code is on my fork

ryansolid commented 6 months ago

Implemented in 0.7.2. Thanks @Brendonovich for the leg work here.