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.

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

Already on GitHub? Sign in to your account

npm run package isn't playing nice without typescript #2951

Closed
kkortes opened this issue Nov 30, 2021 · 11 comments · Fixed by #3562
Closed

npm run package isn't playing nice without typescript #2951

kkortes opened this issue Nov 30, 2021 · 11 comments · Fixed by #3562
Labels
error handling feature / enhancement New feature or request pkg:svelte-package Issues related to svelte-package

Comments

@kkortes
Copy link

kkortes commented Nov 30, 2021

Describe the bug

Running npm run package on a code stack meant to spit out javascript modules (not typescript) gives us the error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte2tsx' imported from /Users/x/Projects/crow/node_modules/@sveltejs/kit/dist/chunks/index6.js

Installning svelte2tsx makes it work, but it bundles .d.ts-files.

Reproduction

  1. Download npm init svelte@next test
    (Skeleton Project > No Typescript > Use ESLint > Use Prettier)
  2. Install cd test; npm install
  3. Run npm run package

Yields the error: Cannot find package 'svelte2tsx'

Logs

No response

System Info

System:
  OS: macOS 11.1
  CPU: (8) x64 Apple M1
  Memory: 648.46 MB / 16.00 GB
  Shell: 5.8 - /bin/zsh
Binaries:
  Node: 14.16.0 - ~/.nvm/versions/node/v14.16.0/bin/node
  npm: 7.20.3 - ~/.nvm/versions/node/v14.16.0/bin/npm
Browsers:
  Chrome: 96.0.4664.55
  Firefox: 94.0.2
  Safari: 14.0.2
npmPackages:
  @sveltejs/adapter-vercel: ^1.0.0-next.23 => 1.0.0-next.23
  @sveltejs/kit: ^1.0.0-next.120 => 1.0.0-next.120
  svelte: ^3.42.6 => 3.44.0

Severity

annoyance

Additional Information

No response

@dominikg
Copy link
Member

The error is unfortunate, maybe we can improve how it works when executing package for the first time.

But you'll want to bundle d.ts even if your library is js, so that users importing it can benefit from the typings.

cc @dummdidumm

@ignatiusmb
Copy link
Member

ignatiusmb commented Nov 30, 2021

It is required to generate type definitions and are encouraged to do so, as mentioned in the docs. You can skip this requirement by disabling emitTypes in the config.

@ignatiusmb ignatiusmb added feature / enhancement New feature or request error handling pkg:svelte-package Issues related to svelte-package labels Nov 30, 2021
@kkortes
Copy link
Author

kkortes commented Nov 30, 2021

The error technically means that npm run package is broken if we installed SvelteKit without typescript support.

The option of disabling emitTypes is nice to have, but the avarage developer shouldn't have to opt out of typescript as a default, if typescript was already neglected upon installation.

At least in my opinion.

Edit: the other approach would be to include svelte2tsx in npm init svelte@next, because it's required anyways?

@dummdidumm
Copy link
Member

dummdidumm commented Nov 30, 2021

Strongly disagree, the average developer should have to opt out of generating type definition files, because publishing a package without type definitions is bad DX for everyone who uses your package and wants to have proper type definitions, be it for type checking against your public API or at least for better intellisense.

What we should do is create a more meaningful error message, but I thought we did that already, so I'm a little surprised that the error presents itself like it does right now.

@kkortes
Copy link
Author

kkortes commented Nov 30, 2021

I agree with the meaningful error message.

I hear what you are saying about type definitions and bad DX.

What I do like with Svelte & SvelteKit is how easy they both are to use, understand and get going with. The behaviour explained here is everything but that. Especially for new eyes coming to try exporting their code as a package.

I'm personally fine with whatever solution we end up with, since I know this caveat now. I'm just raising the alarm for how weird this current behaviour actually is.

@dreitzner
Copy link
Contributor

Maybe we can

  • add a step to the installation (will you use it to generate packages) and add the dependency.
  • make it more prominent in the docs
  • improve the "error" message

@cdellacqua
Copy link

cdellacqua commented Dec 11, 2021

I think npm run package should be smart and add typescript and svelte2tsx if they are not already present, possibly with a prompt asking a Yes/No question like the ones in npm init svelte@next.

I don't think the question should be asked during the npm init ... phase, because it's irrelevant to all developers that are just making a new Web App.

@ignatiusmb
Copy link
Member

They could be using package managers other than npm and there's no easy way to detect those, #1646 (comment). Having a clear error message is the preferred way here, that's how other errors related to configuration are handled as they're only a one-time setup.

@kkortes
Copy link
Author

kkortes commented Dec 12, 2021

Then we should intercept the current error message with something similar to:

”You can't package javascript modules without dependency svelte2tsx. Either install it or disable the requirement for it (highly discuraged): link to docs

@dummdidumm
Copy link
Member

dummdidumm commented Dec 12, 2021

After doing npm init svelte@next and chosing "no" for TypeScript I get

npm run package

> sveltekit-starter@0.0.1 package C:\repos\svelte\sveltekit-starter
> svelte-kit package

> You need to install svelte2tsx and typescript if you want to generate type definitions
Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte2tsx' imported from C:\repos\svelte\sveltekit-starter\node_modules\@sveltejs\kit\dist\chunks\index6.js
Error: You need to install svelte2tsx and typescript if you want to generate type definitions
Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'svelte2tsx' imported from C:\repos\svelte\sveltekit-starter\node_modules\@sveltejs\kit\dist\chunks\index6.js
    at load (file:///C:/repos/svelte/sveltekit-starter/node_modules/@sveltejs/kit/dist/chunks/index6.js:15566:10)
    at async try_load_svelte2tsx (file:///C:/repos/svelte/sveltekit-starter/node_modules/@sveltejs/kit/dist/chunks/index6.js:15553:21)
    at async emit_dts (file:///C:/repos/svelte/sveltekit-starter/node_modules/@sveltejs/kit/dist/chunks/index6.js:15544:15)
    at async make_package (file:///C:/repos/svelte/sveltekit-starter/node_modules/@sveltejs/kit/dist/chunks/index6.js:15263:3)
    at async file:///C:/repos/svelte/sveltekit-starter/node_modules/@sveltejs/kit/dist/cli.js:947:4

So there's the message I expected present at the top of the error stack already - although I agree we can be wordier here with a link to the docs etc. I can't reproduce the case where you are only getting the ERR_MODULE_NOT_FOUND message.

@jcalfee
Copy link

jcalfee commented May 14, 2022

I realize it is in the spirit of typescript to kind-of force everyone into using typescript by not being able to import non-typescript files without that stub, however, that extra wasted time combined with a compromise is the reason why I choose not to tolerate typescript on my side. Java and Eclipse did it right and I really enjoyed that back in the day, it was all optional and did nothing but help you out when you did improve the code. It will sound crazy but please consider my opinion, one has to research that and let it sink in and only then can we have rational view > It is not hard to link FB to the technocrats if you look into it. I know developers are very busy so I don't say much often. Don't their influence rub off is my opinion too though, ts today then what will it be tomorrow? In any event, Thanks svelte-kit! Disabling ts for now would be awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling feature / enhancement New feature or request pkg:svelte-package Issues related to svelte-package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants