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

Imports broken with library updated to svelte-package v2 #9114

Closed
seanlail opened this issue Feb 18, 2023 · 25 comments · Fixed by #9133
Closed

Imports broken with library updated to svelte-package v2 #9114

seanlail opened this issue Feb 18, 2023 · 25 comments · Fixed by #9133
Labels
pkg:svelte-package Issues related to svelte-package

Comments

@seanlail
Copy link
Contributor

seanlail commented Feb 18, 2023

Describe the bug

I updated a lib to use svelte-package v2.

Previously I was using the lib like this:

<script lang="ts">
  import { Button } from "kit-issue-9114";
</script>

<Button>Example</Button>

After updating I get an error from TS that the module does not exist.
I found that if I add the output directory to the import, then it works:

Example:

<script lang="ts">
  import { Button } from "kit-issue-9114/package";
</script>

<Button>Example</Button>

I'm not sure where the issue is here, I used the migration script and have gone through this doc.

Reproduction

https://github.com/seanlail/kit-issue-9114

Logs

Cannot find module 'kit-issue-9114' or its corresponding type declarations.ts

System Info

System:
    OS: macOS 13.1
    CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
    Memory: 3.45 GB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.14.0 - ~/.nvm/versions/node/v18.14.0/bin/node
    npm: 9.3.1 - ~/.nvm/versions/node/v18.14.0/bin/npm
  Browsers:
    Chrome: 110.0.5481.100
    Firefox: 99.0.1
    Safari: 16.2
  npmPackages:
    @sveltejs/adapter-static: 2.0.1 => 2.0.1 
    @sveltejs/kit: 1.7.2 => 1.7.2 
    @sveltejs/package: 2.0.1 => 2.0.1 
    svelte: 3.55.1 => 3.55.1 
    vite: 4.1.2 => 4.1.2

Severity

serious, but I can work around it

Additional Information

No response

@dummdidumm
Copy link
Member

Please provide a reproduction repository (a link to your library is enough for me)

@seanlail
Copy link
Contributor Author

@dummdidumm Done. I just wanted to get the issue number.
If I package the above and then try to use it in a separate project I get the TS error I mentioned above.

@ghostdevv
Copy link
Member

I was unable to reproduce this, running pnpm install ../kit-issue-9114 in an example project to test out your build worked just fine for me

How are you installing your test package?

@sureshjoshi
Copy link

This is a similar problem to what I'm running into:

npm run package and then npm pack, I get this folder structure:

.
├── README.md
├── package
│   ├── Button.svelte
│   ├── Button.svelte.d.ts
│   ├── index.d.ts
│   └── index.js
└── package.json

With TS not really being able to do subpath export handling very well (microsoft/TypeScript#33079) - that package folder (or dist in my case) keeps being required.

@ghostdevv
Copy link
Member

@sureshjoshi Would be interested to see why you are running into issues as well, still unable to reproduce any runtime or type errors

@ghostdevv ghostdevv added the pkg:svelte-package Issues related to svelte-package label Feb 18, 2023
@sureshjoshi
Copy link

sureshjoshi commented Feb 18, 2023

I've worked around most of mine, there are a number of small issues which result in a comedy of errors. I should note, in my case, it's a bit stranger, as I want to allow certain subdirectories and no top-level imports, so not a 1:1 with this ticket.

https://github.com/robotpajamas/svelte-heroicons/tree/2.x
https://discord.com/channels/457912077277855764/1076229817286017055

Actually, for me, what I would like is to flatten the output package, so that the package.json is at the same level as my dist code, but when I package it, I have a similar structure - with the package.json, and then a dist folder.

I'm not sure if that's a configuration change, or if I should copy/paste package.json into the dist folder as per the svelte-package docs suggest, or something else.

And note: The ONLY reason I want to do this is because of the poor TS support for subpaths, as per the attached issue above. If I could map by subpaths and have them show up in intellisense, I'd be perfectly happy.

@ghostdevv
Copy link
Member

The svelte package behaviour changed so that it no longer will generate a package.json in the dist (formerly package) folder hence the breaking change of v2

I personally haven't run into poor support of paths in ts like you describe so can't help there unfortunately

@sureshjoshi
Copy link

sureshjoshi commented Feb 18, 2023

Yep, that's what I understood from the #8825 discussion (or maybe the svelte-package docs).

So, looking to try and re-create that behaviour for this icons package.

Or in general, figuring out some way to flatten away the intermediate dist folder. That would clean up everything for me. Seems to be automatic though.

@dummdidumm
Copy link
Member

Ah yeah that's a bummer that TS is not playing ball (it would with TS 5.0 and "moduleResolution": "bundler", but we can't expect that from people).
Ok so this sounds kinda stupid because this was what @sveltejs/package version 1 did, but: copy over the package.json into your dist folder and publish from there. You still need to manually adjust your exports (so basically remove the /dist part from all paths), but then it should work like before.

@sureshjoshi
Copy link

I've been trying this today, but what does "publish from there" mean in this context.

Like, just jump into dist and publish there? Is it that simple?

@sureshjoshi
Copy link

... My head is going to explode. Yep, it was that simple 🤦🏽

Thanks @dummdidumm !

@dummdidumm
Copy link
Member

dummdidumm commented Feb 18, 2023

Yes:

  1. copy package.json into dist folder (with adjusted export paths etc, so that the /dist part is removed)
  2. cd dist
  3. npm publish

(part 1 could be automated - the copy thing, not sure about the /dist adjustment thing. Given that it's not uncommon we should probably have an option for that in svelte-package)
(yes this sounds like @sveltejs/package version 1 but it's not the same)

@benmccann
Copy link
Member

it would with TS 5.0 and "moduleResolution": "bundler", but we can't expect that from people

It also works with node16, but of course that's also painful to use since it requires file extensions. I feel like it'd be fair to suggest people use bundler instead of outdated and buggy implementations, but it'd certainly be easier to suggest that if it were already available 😆

dummdidumm added a commit that referenced this issue Feb 20, 2023
and explain that stuff in the docs
closes #9114
@dummdidumm
Copy link
Member

FYI I found a better solution to this which keeps us from introducing a fragile copy function: (ab)using the typesVersions feature from TypeScript to replicate the export path mappings. It's explained in the linked PR's docs which are hopefully up on the main Kit site soon. The Button.svelte example in this issue would be written as follows:

{
  "typesVersions": {
    ">4.0": {
      "Button.svelte": ["./dist/Button.svelte.d.ts"]
    }
  }
}

svelte-migrate will be updated to migrate this automatically, so it's probably easiest to run the migration script again on your libraries to get the typesVersions addition to your package.json, copy it out, revert the rest, copy it back in.

dummdidumm added a commit that referenced this issue Feb 20, 2023
and explain that stuff in the docs
closes #9114
@sureshjoshi
Copy link

sureshjoshi commented Feb 21, 2023

After doing this, has anyone lost TS intellisense in VSCode? I can't seem to get VSCode to see my svelte or .ts modules after I've used typeVersions.

When I use the instructions above to publish from the dist/ dir - everything works PERFECTLY.

As soon as I try to use typeVersions, and have subpaths, I suddenly lose any and all intellisense. I've refreshed language servers, restarted VS Code, restarted my computer, anything I can think of.

@dummdidumm
Copy link
Member

@sureshjoshi can you give a more concrete example? I just pulled down your master branch of @robotpajamas/svelte-heroicons and these work for me:

import {} from '@robotpajamas/svelte-heroicons';
import { AcademicCapIcon } from '@robotpajamas/svelte-heroicons/solid';
import { AcademicCap } from '@robotpajamas/svelte-heroicons/solid/AcademicCap';
import Cap from '@robotpajamas/svelte-heroicons/solid/AcademicCap/AcademicCap.svelte';

Do you mean that there's no path autocompletion? E.g. you do @robotpajamas/svelte-heroicons/solid/A and don't get AcademicCap as a suggestion?

@sureshjoshi
Copy link

Hey @dummdidumm - if this is the version where I removed typeVersions, then intellisense works again.

I'll try to make a breaking branch shortly, but the path completion works - but I get all the red squiggles, even though path completion works, and the code compiles/transpiles/runs just fine.

@dummdidumm
Copy link
Member

dummdidumm commented Feb 21, 2023

I ran the migration script on your master branch and used the outcome (including the typesVersions), which worked for me. Happy to look into a breaking branch.

@sureshjoshi
Copy link

sureshjoshi commented Feb 21, 2023

https://github.com/RobotPajamas/svelte-heroicons/tree/sketchy-ts-support

Workflow is:

  • Clone repo
  • pnpm package
  • pnpm pack (same problem happens when I publish to npm as well, this is just a faster test)

From another repo where I use the icons:

  • pnpm install ~/path/to/robotpajamas-svelte-heroicons-2.0.15-rc.3.tgz

And then I get this in VSCode:

Screenshot 2023-02-21 at 08 24 40

If I'm not mistaken, the only real changes are using typeVersions for subpath imports. Maybe there is some default index.d.ts importing that would happen when I'm using the flat (no dist) publishing, but I'm not quite certain.

Note: I also did whatever TS/Svelte server restarts I could, reboots, etc - still no types.

@sureshjoshi
Copy link

Oh, interesting, after npx migrate, it enumerates ALL of the files and doesn't use the wildcard format. Fascinating, I might need to look into this more.

From the docs, I was under the impression that dist/* would basically re-direct everything. https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#version-selection-with-typesversions

@dummdidumm
Copy link
Member

Thanks, here's the things I found:

  • you are referencing dist/* but your package is output to package. The migration script keeps the old output folder because we don't know if there are other things that might reference them (that's the -o package you see in your package script)
  • the key is wrong. you have to read the key as "this is the import path the user types" and the array as "this is where TS looks for the types"

So a more correct version would look like this:

"typesVersions": {
    "*": {
      "*": [
        "package/*"
      ]
    }
  }

That works, but now you export more than you like (judging from your exports map), because you want only the index files exposed. So the final version is:

"typesVersions": {
    "*": {
      "index": [
        "./package/index.js"
      ],
      "mini": [
        "./package/mini/index.d.ts"
      ],
      "mini/*": [
        "./package/mini/*/index.d.ts"
      ],
      "outline": [
        "./package/outline/index.d.ts"
      ],
      "outline/*": [
        "./package/outline/*/index.d.ts"
      ],
      "solid": [
        "./package/solid/index.d.ts"
      ],
      "solid/*": [
        "./package/solid/*/index.d.ts"
      ]
    }
  }

@sureshjoshi
Copy link

Hmm, interesting - I didn't use the migration script, so I don't have package anywhere (that I know of). This is actually my first npm package, so nothing to migrate from 😄

However, I will try what you suggested re: typesVersions. I've tried about 100 permutations over the last week, so hard to recall what I've tried and not tried 😮‍💨

Looking at my sample branch, I actually think I made a mistake. The attempt I thought would work should look like this (still didn't work, so 🤷🏽).

"typesVersions": {
    "*": {
      "*": [
        "dist/*"
      ]
    }
  },

In any case, will try your version later today and report back!

@dummdidumm
Copy link
Member

huh, nevermind about -o package then, I could've sworn I saw it somewhere. Replace all /package occurences in my snippets with /dist then 😄

@janosh
Copy link
Contributor

janosh commented Mar 13, 2023

With tomorrow's TS v5 release, does the recommended fix change? Would prefer not to have to litter my package.jsons with "typesVersions".

@janosh
Copy link
Contributor

janosh commented Mar 13, 2023

Btw, getting 2 warnings after pnpm add -D typescript@beta:

devDependencies:
- typescript 4.9.5
+ typescript 5.0.0-beta

 WARN  Issues with peer dependencies found
.
├─┬ @sveltejs/package 2.0.2
│ └─┬ svelte2tsx 0.6.7
│   └── ✕ unmet peer typescript@^4.9.4: found 5.0.0-beta
└─┬ svelte-preprocess 5.0.1
  └── ✕ unmet peer typescript@"^3.9.5 || ^4.0.0": found 5.0.0-beta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:svelte-package Issues related to svelte-package
Projects
None yet
6 participants