stronger dynamic env types #7735
Conversation
🦋 Changeset detectedLatest commit: 39552e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
packages/kit/src/core/env.js
Outdated
| .filter((k) => valid_identifier.test(k)) | ||
| .map((k) => `\t\t${k}: string;`); | ||
|
|
||
| properties.push(`\t\t[key: string]: string | undefined;`); | ||
| properties.push( | ||
| `\t\t[key: \`${env.prefix}\${string}\`]: ${id === 'public' ? 'string | undefined' : 'never'}` |
There was a problem hiding this comment.
instead of pushing never, shouldn't we just leave out this key/value pair?
There was a problem hiding this comment.
Agree with not using never, but we should still include this because if we leave this out, the interface will still have the [key: string]: string | undefined signature added below. In that case, accessing variables starting with env.publicPrefix from the /dynamic/private module will return the string | undefined union, which is not as nice compared to only (currently never, changing to) undefined as this will get caught on compile/lint.
|
It's not wrong per se, but not narrow enough in that the public dynamic types allow all variables by using |
packages/kit/src/core/env.js
Outdated
| properties.push( | ||
| `\t\t[key: \`${env.prefix}\${string}\`]: ${ | ||
| id === 'public' ? 'string | undefined' : 'undefined' | ||
| };` | ||
| ); | ||
|
|
||
| if (id === 'private') { | ||
| properties.push(`\t\t[key: string]: string | undefined;`); | ||
| } |
There was a problem hiding this comment.
I think it might make more sense to combine this into
if (id === 'private') {
// push [key: string] signature
} else {
// push [`PUBLICPREFIX_${string}`: string] signature
}(whats the minimum required TS version for this btw? I think it's in our range, right?)
There was a problem hiding this comment.
I can see that, but it'll look more like this
if (id === 'private') {
// push { [`PUBLICPREFIX_${string}`: string]: undefined } signature
// push { [key: string]: string | undefined } signature
} else {
// push { [`PUBLICPREFIX_${string}`: string]: string | undefined } signature
}I believe it is within our range, minimum version for this was 4.4 where template literal in index signature was added
There was a problem hiding this comment.
Mhm for some reason I thought that private dynamic also includes the public variables for convenience - nevermind then.
Prefixes the dynamic type index signature with
config.env.publicPrefixPlease don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0