Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
2 tasks done
Brendonovich opened this issue Jan 7, 2024 · 5 comments 路 May be fixed by TanStack/form#631
Closed
2 tasks done

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

Brendonovich opened this issue Jan 7, 2024 · 5 comments 路 May be fixed by TanStack/form#631
Labels
enhancement New feature or request

Comments

@Brendonovich
Copy link
Contributor

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the 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/@[email protected][email protected][email protected][email protected][email protected]/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/@[email protected][email protected][email protected][email protected][email protected]/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/@[email protected][email protected][email protected][email protected][email protected]/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/@[email protected][email protected][email protected][email protected][email protected]/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
@Brendonovich Brendonovich added the bug Something isn't working label Jan 7, 2024
@ryansolid
Copy link
Member

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.

@RanolP RanolP mentioned this issue Jan 11, 2024
Closed
@ryansolid ryansolid changed the title [Bug?]: .d.ts files are not provided by @solidjs/start [Feature?]: @solidjs/start should provide .d.ts Jan 25, 2024
@ryansolid ryansolid added enhancement New feature or request and removed bug Something isn't working labels Jan 25, 2024
@ryansolid
Copy link
Member

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
Copy link

jakst commented Mar 7, 2024

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:

  • Turn on all the strictest settings of typescript and make sure solid-start passes them. I think this is what causes the errors to the user, that they have a more strict typescript config than solid-start has.
  • Try publishing to jsr.io as well. It's very new and very exciting. You wouldn't have to add a build step, because the registry handles that part. Maybe too unconventional for some, but I would very much consider getting solid-start from there if it alleviated this issue.
  • Add a build step. I know the team doesn't want to, but honestly this is the same as sacrificing UX for DX, just on a lower level (app author DX vs framework author DX). Thousands of developers feel the pain from this setup at the moment.

Just my 5 cents as a solid-js user since 3 years who really wants solid-start to succeed! 馃檪

@Brendonovich
Copy link
Contributor Author

Brendonovich commented Mar 7, 2024 •
edited

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
Copy link
Member

ryansolid commented Mar 9, 2024 •
edited

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

@Brendonovich Brendonovich mentioned this issue Mar 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: add solid export condition to @tanstack/solid-form Brendonovich/form
3 participants
@ryansolid @jakst @Brendonovich