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

Reserve config export #7845

Closed
Rich-Harris opened this issue Nov 28, 2022 · 18 comments · Fixed by #7878
Closed

Reserve config export #7845

Rich-Harris opened this issue Nov 28, 2022 · 18 comments · Fixed by #7878
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

We've alluded to a vague plan to support route-level deployment config in the future — something like this...

export const config = {
  runtime: 'edge'
};

...where config is a freeform (but serializable) object that gets passed to adapters so that they can use it to customise the deployment.

At the moment, you can export anything from a +(layout|page)(.server)?.js file. This means that adding a new known export would be a breaking change, as someone might already be doing export const config = {...} for some other reason.

Describe the proposed solution

Reserve the config name, i.e. throw an error if a route file exports a binding called config.

Alternatives considered

Prevent any unknown exports. This is safer, but might feel unduly restrictive.

Importance

would make my life easier

Additional Information

No response

@Rich-Harris Rich-Harris added this to the 1.0 milestone Nov 28, 2022
@Conduitry
Copy link
Member

Are there particular reasons for allowing other unknown exports? I feel like that would be a good idea. We're already reserving all + files as being completely under Kit's rules, and we're disallowing unknown ones of those, and so allowing unknown exports within them seems less preferable to me.

@dummdidumm
Copy link
Member

I'm pretty use people use other exports to deduplicate some stuff, so I'm wary of adding a strict "nope" here.

The alternative for this is configuration: For every feature that would be breaking, we create a config option which makes you opt in to the new export handling, and those are removed for version 2 at which point everyone has to migrate.

@Conduitry
Copy link
Member

Deduplicate what sort of stuff? Couldn't that be exported by another file, which is then imported (and/or re-exported) by +page.js?

@Rich-Harris
Copy link
Member Author

I've definitely done this sort of thing in the past (and reused read elsewhere):

export function read(slug) {
  return fs.readFileSync(`content/${slug}.md`, 'utf-8');
}

export function load({ params }) {
  return {
    html: marked(read(params.slug))
  };
}

It absolutely could be exported from a different file. I don't think there's ever a reason you'd need to export something custom from a route file. But it could be mildly cumbersome if you were (for example) exporting something purely for the sake of unit testing it.

I am firmly in the Undecided camp.

@cdcarson
Copy link
Contributor

But it could be mildly cumbersome if you were (for example) exporting something purely for the sake of unit testing it.

Yes, but not "mildly," IMO. For example, I have a wrapper that allows me to write and test action functions according to my own lights about return vs throw. I use it like this...

export const signIn = async (event: RequestEvent): Promise<Redirect> => {
  // blah blah blah
};
export const actions: Actions = {
  default: wrapAction<SignInFormData>(signIn)
};

Disallowing this would either mean (a) making another file or (b) writing less concise, more convoluted functions/tests.

I'd be happy to add an underscore to the exported wrapped function: _signIn, if that helps.

@gterras
Copy link

gterras commented Nov 30, 2022

Some advanced (and very useful) plugins like https://github.com/HoudiniGraphql/houdini also take advantage of exporting stuff there. Overall if feels unnecessarily restricting and will close the door to many potential ingenious plugins later on (which is sad considering plugins may become very important for svelte-kit at some point).

@Rich-Harris
Copy link
Member Author

Allowing underscore-prefixed exports per @cdcarson's suggestion would allow plugins/libraries to do whatever they needed though, right? If anything the existence of those sorts of integrations makes it much more important that we're able to distinguish between framework space and user space. Cc @AlecAivazis

@dummdidumm
Copy link
Member

cc @jycouet

@jycouet
Copy link
Contributor

jycouet commented Nov 30, 2022

In houdini our +page.ts look like this (and only this):

import type { MyPageQueryVariables as V } from './$houdini'

export const MyPageQueryVariables: V = (event) => {
	return { id: event.params.id }
}

In the background, houdini vite plugin generates something like:

import { load_MyPageQuery } from '$houdini'

import type { PageLoad } from './$types'

export const load: PageLoad = async (event) => {
	return load_MyPageQuery({ event, variables: { id: event.params.id } })
}

Houdini users get to choose the syntax they want.
The idea behind the top version is to focus as much as possible on the business logic.


It's just an example, we also provide other hooks like beforeLoad, afterLoad, onError.

@arxpoetica
Copy link
Member

With _ escape hatches, we steer users to use + files the "kit way." Encapsulation around what kit + files are intended for seems a good idea, and future proofing them w/ an escape hatch is likely good design.

@AlecAivazis
Copy link

Out of all of the proposed options, the _ prefix is my favorite. Discovering the value embedded in config is a lot harder for tools to support and would likely mean all sorts of limitations to ensure that it's easily statically analyzable. Preventing unknown exports all together impacts the reusability of a route in general, not just the lives of plugin authors (more on this at the end).

That being said, I do kind of want to challenge if this is necessary at all. Like @jycouet and @gterras mentioned, Houdini adds a lot of special values to the salad of magic names in +page files that often mean users don't have any of kit's standard ones. They often don't even have a +page file but those situations don't really impact this conversation. It's not the end of the world if every exported value from a file starts with _ but it definitely feels funny since _ usually means its a private value (although that's pretty arbitrary and something I'm sure I would get over). If anyone is curious, here is an overview of the kinds of things we support.

If anything the existence of those sorts of integrations makes it much more important that we're able to distinguish between framework space and user space

Is this because of a technical reason or a human one (documentation, education, etc)?

While I can't speak with 100% certainty, I haven't heard of anyone being confused by this mixing. If anything, i think it creates a rather nice narrative where users are free to mix and match Kit and Houdini without juggling multiple files or different conventions. In some sense, making sure we add features in a kit-compatible way is our job as authors of a meta-layer. A concrete example of this is when we added the "houdini_" prefix to houdini_load which can be set to a graphql query, causing the plugin to generate everything else. If there is a technical reason for that to be _houdini_load then I think it's a totally fine compromise.

Couldn't that be exported by another file, which is then imported (and/or re-exported) by +page.js?

The goal for us is to dramatically reduce the amount of boilerplate that a user has to maintain by assuming as much as possible. If the solution is to move all of that out of the +page file and into a houdini-specific file, we'll have to come up with a new special prefix which feels like an unnecessary complication. For some additional context, we are using the + prefix for non-js files to make them feel kit-native. On top of that, we will still be responsible for "faking" the file for the generated manifests (I wrote a discussion about this here). Overall this feels like closing a door that we are holding open to keep some semblance of integrating with kit.

@Rich-Harris
Copy link
Member Author

I'm not familiar with Houdini's innards, but from a quick glance it sounds like you're essentially rewriting the +page.js file to account for things like beforeLoad and +page.gql etc? If so, then you shouldn't actually be affected by this at all (unless there were a TypeScript plugin that validated your +page.js as you were writing it, etc, but that could be made configurable). It's a runtime check rather than static analysis

@AlecAivazis
Copy link

AlecAivazis commented Nov 30, 2022

So there are a few things at play here.

When I first started building the vite plugin, I wanted to avoid the static analysis by importing the file from within the transform hook so we could support any language that vite was configured to work with. Unfortunately, I ran into an issue ( vitejs/vite#6810) that made that approach not possible so I compromised by analyzing the file statically. This is clearly not ideal but short of running vite on each file separately when processing/building and then importing the resulting file, there isn't really a work around I could think of. Until then, static analysis is the only option but I don't think that's a good reason to prevent plugin authors from ever being able to import/use the +page files for custom config. Once that issue gets fixed, it creates a very natural way for someone to build powerful transformers for a route.

Regardless of the plugin's current implementation, our magic values need to be exported because the types that we generate to fill the same role was the ./$types import need to include the houdini_load values as well as what's added to the load payload with the beforeLoad and afterLoad hooks.

edit3: consolidated, reworded, and removed a lot of context. i want to avoid this becoming a houdini-specific conversation since I think the original issue is very general and focused on adapters which have a very different set of goals and constraints. I'd love to go deeper into the notion of a SvelteKit plugin and what it should be allowed to do but I think this is the wrong place for that since it's definitely a post-1.0 thing. Maybe #6708 is more appropriate?

@Rich-Harris
Copy link
Member Author

Thanks — I'm still unclear on whether entry points have the custom exports after the Houdini transform is done with them though?

@AlecAivazis
Copy link

AlecAivazis commented Dec 1, 2022

Yea they do but the primary reason is laziness/simplicity . However, once that issue is fixed, I think removing the custom exports would create a problem since the plugin wouldn't be able to import the file and see if anything special was exported

@dummdidumm
Copy link
Member

But doesn't the plugin do its magic before the file is seen by SvelteKit? So the plugin could (all before the file is seen by SvelteKit)

  1. load the file and look at its contents
  2. do magic based on special houdini exports and transform the +page.js to a regular +page.js with a load function (I guess that's what's happening under the hood?) and remove the other exports (or prepend them with an underscore)
  3. hand the transformed file off to SvelteKit, which is fine with the result

The typings are separate, at least on our side they work through simple static analysis and doing some simple AST transforms, resulting in some generated types - no validation of forbidden exports there.

@AlecAivazis
Copy link

AlecAivazis commented Dec 1, 2022

Yes that's how things work now and likely how things would have to stay if exports were removed. Like I said, the hurdle I mentioned only pops up once vitejs/vite#6810 is addressed since it muddles the processing order a bit: a plugin that generates code based on custom exports might want to call this.load inside of the plugin to discover a file's exports instead of relying on static analysis which has some downsides (see below). This mean that they would only have access to the transformed version. Looking for the prefixed exports is not that bad which is why I said I thought it would be a perfectly fine compromise. It's only a major issue if the exports were removed all together. Again, this point only matters once an almost 1 year old issue on vite is addressed. The only reason I brought it up is to highlight a potential edge case that might pop up when its merged.

In general, I'm not a huge fan of going against the grain when it comes to constraints imposed by Kit. I think I'd rather require users to add the _ prefix to our magic exports to ensure that our plugin integrates with any tools that pop up in the community and to avoid confusing people who are seeing something that shouldn't be allowed according to the kit docs.


I still think this is part of a larger conversation about what the development experience for a SvelteKit plugin is like. The code that performs the static analysis to look at exports in +page files would have to be duplicated across every plugin and has some obvious hurdles:

  • people have to bring our their parser (or use vite's acorn) so which makes it tricky to support arbitrary languages that the user might have enabled with other plugins (or versions of typescript if they fall behind)
  • if the user has done something unexpected with imports, local variables, envars, etc that break our heuristics then we're out of luck
  • they have to check for overlapping things like export const magicExport = () => ... as well as export function magicExport.

Clearly these aren't that bad to support or work around but it does leave me with a feeling that there's an opportunity for a smoother experience for developers looking to build Kit-specific plugins. These plugins have common needs which are out of scope for treating it like a generic vite plugin, like looking at the exported values of +page files or transforming files that might not actually exist in the filesystem. I tried to go into a little more detail on this in that discussion if anyone is interested.

@AlecAivazis
Copy link

AlecAivazis commented Dec 1, 2022

Just to reiterate - I dont think this conversation is super relevant to the original issue. Nor do I expect this to be resolved before 1.0 can get released. I understand the want to group as many breaking changes as possible before 1.0 and i would feel very bad if this conversation was blocking the release. To me, it seems like the _ prefix provides all of the future-proofing required and leaves the rest of the conversation for another time.

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.

8 participants