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

Remove relative path references in adapters #2569

Closed
benmccann opened this issue Oct 7, 2021 · 6 comments
Closed

Remove relative path references in adapters #2569

benmccann opened this issue Oct 7, 2021 · 6 comments
Milestone

Comments

@benmccann
Copy link
Member

benmccann commented Oct 7, 2021

Describe the bug

The best idea I have so far is that instead of writing the server output to .svelte-kit/output/server, we could try writing it to something like node_modules/@sveltejs/user-app (or maybe src/node_modules like Sapper does) with a package.json file:

		{
			"name": "@sveltejs/user-app",
			"version": "0.0.1",
			"private": true,
			"type": "module",
			"main": "app.js"
		}

The relative path is stopping adapter authors from using the types

I also think that after this is fixed that we can use Vite to do a single build pass instead of doing a second build step with esbuild in the adapters

Reproduction

For example:

// TODO hardcoding the relative location makes this brittle

We should make sure @jthegedus can consume the types in the firebase adapter after this is fixed

Logs

No response

System Info

`master`

Severity

annoyance

Additional Information

No response

@benmccann benmccann added this to the 1.0 milestone Oct 7, 2021
@Mlocik97
Copy link
Contributor

Mlocik97 commented Oct 8, 2021

I don't think it's good idea to write this to src/node_modules (it caused problems in Sapper). I agree we need to solve those relative paths, but there should be maybe better way?

@benmccann
Copy link
Member Author

What problems did it cause in Sapper?

@Mlocik97
Copy link
Contributor

Mlocik97 commented Oct 8, 2021

sveltejs/sapper#1696

for example...

there is at least 10 similiar issues on Sapper Github.

One even more critical, about bundlers, as it's problematic to process src/node_modules/, (I'm lazy for searching issues)

Also I think Rich was talking about how he wanted to get rid of this, as beginners were often confused with this folder. It's even why in Kit tried new approach with .svelte-kit/ folder

@benmccann
Copy link
Member Author

That issue sounds like people didn't like using src/node_modules/images (as is done in the template). SvelteKit doesn't do that and instead uses Vite aliases, so I don't think that issue is relevant.

Still, we should probably put it directly in node_modules as I think src/node_modules was confusing to people. The Sapper FAQ seems to suggest adding a new dependency would nuke it if put directly in node_modules, but that doesn't sound right to me. We should test

@benmccann benmccann added pkg:adapter-vercel Pertaining to the Vercel adapter and removed pkg:adapter-vercel Pertaining to the Vercel adapter labels Oct 8, 2021
@Mlocik97
Copy link
Contributor

Mlocik97 commented Oct 8, 2021

what about putting node_modules to .svelte-kit ? I think having .svelte-kit/node_modules/ is best solution, bcs, it's not wiped when installing new dep,... it's not inside src/so it doesn't confuse people as it is in hidden generated folder.

@benmccann
Copy link
Member Author

No longer an issue:

import { App } from 'APP';

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants