[breaking] rename context param of load to stuff

[dummdidumm]

Closes #984

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 997d795

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

[benmccann]

Awesome. Thanks!

It might not be a bad idea to log a warning if we encounter someone returning context from their load function. We're getting close enough to 1.0 now and Kit has almost as many users as Sapper that it might be nice to help folks out a bit with stuff like this eventhough we're still in beta

[Conduitry]

Do we have the ability to do that in dev mode only?

Alternatively, is there a reasonable way to check for that only during SSR? It is a nice warning to be able to give people, but I don't want to be sending extra bytes to browsers.

Also alternatively, should it always be a warning/error to return any unknown keys from the load function, long-term, not just this one that we're renaming?

[benmccann]

Yeah, we can do it dev mode only. Check the mode from $env or import.meta.env.DEV and Rollup/Vite will remove it from production build

[dummdidumm]

Added the warning. Using dev from ./app/env here, which wasn't used anywhere else yet (only for user-facing-code), hope that doesn't have any bad implications.

[dummdidumm]

Well, it does apperently, tests are failing now because env is not set in tests. What now?

[bluwy]

Would context still work in prod? If not I think we should make a hard error here.

But regarding the error, I believe it's because import.meta.env is non-standard in nodejs so it would resolve to undefined, hence the error. A workaround is to shim it with import.meta.env = { /* ... */ } at the top of this file, though we need to make sure we do that for tests only. Using process.env.NODE_ENV may be fine for this as well.

[benmccann]

I'd expect it to be defined in the integration tests. I think the issue is around this being unit tested. Rather than modify this to shim it, I wonder if we could move the check to another location that's called in integration tests, but not unit tests.

[dummdidumm]

This sounds wrong to me. I picked this method as it seems the central location to validating the load response. Also we probably don't have this check in for long, we could remove it at 1.0 and before that shimming it sounds like a pragmatic solution in the meantime

[benmccann]

Ok. That makes sense

[dummdidumm]

Can we safely use import.meta.env in all Kit code that is run by the user? If so, is the failure of the tests only a hint for us to provide these import.meta.env definitions?

[dummdidumm]

I failed to come up with a way to set this that is both tree shakeable and doesn't require us to introduce esm import mocking into the unit tests, so I just left the error in - it will be shipped now every time (sorry @Conduitry ). I think this is okay since we will throw it out in a couple of weeks anyway.