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

fix: disallow exporting actions from a +layout.server file #10046

Merged
merged 5 commits into from
Jun 3, 2023

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented May 27, 2023

Found out we can export actions in a layout file but it isn't mentioned anywhere in the docs.
I don't think we're suppose to export actions from a layout file. #10046 (comment)

This PR changes the list of valid exports to disallow the actions export from a +layout.server file (matching the language tools behaviour).

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Sorry, something went wrong.

@changeset-bot
Copy link

changeset-bot bot commented May 27, 2023

🦋 Changeset detected

Latest commit: 58cd9c5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@eltigerchino eltigerchino changed the title fix: mention +layout.server.js in the form actions docs docs: mention +layout.server.js in the form actions docs May 27, 2023
@eltigerchino eltigerchino changed the title docs: mention +layout.server.js in the form actions docs docs: mention +layout.server.js in the form actions section May 27, 2023
@david-plugge
Copy link
Contributor

I don´t think you can do that. Vscode gives me this error:

Invalid export 'actions' (valid exports are prerender, ssr, csr, trailingSlash, config, load, or anything with a '_' prefix

@eltigerchino
Copy link
Member Author

Huh. That's weird. It's suppose to be valid based on the code here:

const valid_layout_server_exports = new Set([...valid_layout_exports, 'actions']);

and it's tested here
check_error(() => {
validate_layout_server_exports({
answer: 42
});
}, "Invalid export 'answer' (valid exports are load, prerender, csr, ssr, trailingSlash, config, actions, or anything with a '_' prefix)");

@eltigerchino
Copy link
Member Author

Not sure if we need to update language tools to allow actions in layout or update kit to disallow actions in layout.
https://github.com/sveltejs/language-tools/blob/98b925617bbdbf6e9e51a1d7e229d0f27c2337cc/packages/typescript-plugin/src/language-service/sveltekit.ts#L280-L281

@david-plugge
Copy link
Contributor

I wonder how actions in layout files would work, specifically how you would call them from the client (what to put in the form action attribute?). Page and layout actions could collide, how would the server choose between src/routes/#layout.server.ts#actions.signin and src/routes/#page.server.ts#actions.signin ?

@benmccann benmccann requested a review from Rich-Harris May 30, 2023 15:24
@elliott-with-the-longest-name-on-github
Copy link
Contributor

I... would expect you shouldn't export actions from layout components? I have no idea what URL they would / should live at.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was signed with the committer’s verified signature.
SimenB Simen Bekkhus
@eltigerchino eltigerchino changed the title docs: mention +layout.server.js in the form actions section fix: disallow exporting actions from a +layout.server file May 31, 2023
@eltigerchino eltigerchino merged commit 3b41d65 into master Jun 3, 2023
@eltigerchino eltigerchino deleted the s3812497-patch-1 branch June 3, 2023 00:18
@github-actions github-actions bot mentioned this pull request Jun 3, 2023
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 this pull request may close these issues.

None yet

3 participants