-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
prevent third-party types in twoslash snippets #3309
prevent third-party types in twoslash snippets #3309
Conversation
**Overview** In the React code for the site, we're using third-party types from the `@types/*` packages. This is good, but it seems that these types are unintentionally also getting included as part of the compilation of our twoslash snippets in our docs pages. This change makes it so that these types are _not_ included in our twoslash snippets by default. We can still get them if we need them in a particular snippet, by specifying the `@types` compiler option within the snippet itself. In the current docs, though, we don't need them most of the time; I counted only 2 snippets where this is necessary (out of the ~800 we produce during the Gatsby build). This general change started when I noticed a [specific issue on the "Object Types" page](https://www.typescriptlang.org/docs/handbook/2/objects.html#the-array-type) (more detail below). In addition to fixing that specific issue, this change offers a slight "correctness" improvement where we use the `console` API in our snippets, since our "quick info" boxes will now show types from the default TS DOM lib, rather than ones from `@types/node`. This change also offers a performance improvement of the Gatsby build; here are some times from my machine (running `gatsby clean` and deleting the twoslash cache directory between runs): ``` // before $ time pnpm build-site 591.06s user 82.26s system 269% cpu 4:10.10 total // after $ time pnpm build-site 333.84s user 41.71s system 307% cpu 2:02.18 total ``` Since this change is only to our Gatsby config, it shouldn't affect the Playground (it will actually make our docs snippets more consistent with the Playground, since we don't bring in third-party types by default there). **Background** In ["Object Types"](https://www.typescriptlang.org/docs/handbook/2/objects.html#the-array-type), we demonstrate what the `Array` type looks like in the default TS lib by defining `interface Array<Type>` in a snippet. However, we have the unexpected errors TS2317 and TS2428, which have to do with incorrectly extending an existing interface. Weirdly, if we open the corresponding Playground link, the Playground doesn't show these errors. What's going on? It turns out that, despite our specifying `@noLib: true` in the snippet, when we compile with twoslash, we're "accidentally" ending up with `@types/node` in the compilation, because it's in our own `node_modules` directory. This explains why we don't see the errors in the Playground: there, we use the "in-memory filesystem" provided by `createSystem` in `typescript-vfs`, but during the Gatsby build, we use the `createFSBackedSystem`, who delegates to the real filesystem, so that when TS goes looking for `node_modules/@types` [per the default `typeRoots` behavior](https://www.typescriptlang.org/tsconfig/#typeRoots), it will discover the one from our own project. Specifically, our errors are because `@types/node` contains a type definition for `Array<T>`, but in our snippet, we try to define `Array<Type>` with a differently-named type parameter. Therefore, we _could_ work around the error by simply changing `Type` to `T`. However, since our intention with `@noLib: true` was that `Array` would have no other declarations, the current behavior seems wrong, and therefore we should find a better solution. It actually gets a little weirder, since `@types/node` contains a triple-slash reference to the default ES2020 lib, so now we're _really_ getting the lib who we asked not to get :) furthermore, it's the ES2020 lib, even though our `target` is set to ES2016 in the twoslash default options. I discovered this behavior by stepping through the twoslash logic in the debugger and inspecting things like: ``` ls.getProgram().getSourceFiles() ls.getProgram().getAutomaticTypeDirectiveNames() ls.getTypeDefinitionAtPosition('<filename of snippet>', <character position where we reference a type>) ``` **What docs actually change with this change** I did diffs of the Gatsby output `public` directory to get a concrete answer to this. I found only 2 places where losing third-party types results in a "decrease in correctness," which we can fix by specifying `@types` in the snippet. Then there's the aforementioned change to use the `console` types from the DOM lib (either an "increase in correctness" or a no-op :)). Like mentioned above, our "accidental" inclusion of `@types/node` resulted in the transitive inclusion of the ES2020 lib. When we lose this, we fall back to our explicitly-configured `target` of ES2016. This would be a problem for snippets who use APIs from those newer ES versions; seems like there's only 1 place where this happens, though: in ["Basics"](https://www.typescriptlang.org/docs/handbook/2/basic-types.html#non-exception-failures), when we use `toLocaleLowerCase`: ``` // from lib/lib.es5.d.ts toLocaleLowerCase(locales?: string | string[]): string; // from lib/lib.es2020.string.d.ts toLocaleLowerCase(locales?: Intl.LocalesArgument): string; ``` Since we were getting ES2020 without knowing it anyway, we might as well make it our default `target`.
@microsoft-github-policy-service agree |
Sorry for the novel; I figured this was an unusual change so it warranted some explanation. However, after writing everything up, I see that we already discussed this a little bit in #3165, so a TL;DR for this change would be "doing the same thing to the Gatsby build that we did to the twoslash tests in #3165" 🙂 If we wanted to use ES2023 as our |
I'd just delete it, honestly |
CI went from 23 minutes to 13 minutes, awesome. |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://victorious-plant-05c166c10-3309.centralus.5.azurestaticapps.net |
Overview
In the React code for the site, we're using third-party types from the
@types/*
packages. This is good, but it seems that these types are unintentionally also getting included as part of the compilation of our twoslash snippets in our docs pages.This change makes it so that these types are not included in our twoslash snippets by default. We can still get them if we need them in a particular snippet, by specifying the
@types
compiler option within the snippet itself. In the current docs, though, we don't need them most of the time; I counted only 2 snippets where this is necessary (out of the ~800 we produce during the Gatsby build).This general change started when I noticed a specific issue on the "Object Types" page (more detail below). In addition to fixing that specific issue, this change offers a slight "correctness" improvement where we use the
console
API in our snippets, since our "quick info" boxes will now show types from the default TS DOM lib, rather than ones from@types/node
.This change also offers a performance improvement of the Gatsby build; here are some times from my machine (running
gatsby clean
and deleting the twoslash cache directory between runs):Since this change is only to our Gatsby config, it shouldn't affect the Playground (it will actually make our docs snippets more consistent with the Playground, since we don't bring in third-party types by default there).
Background
In "Object Types", we demonstrate what the
Array
type looks like in the default TS lib by defininginterface Array<Type>
in a snippet. However, we have the unexpected errors TS2317 and TS2428, which have to do with incorrectly extending an existing interface. Weirdly, if we open the corresponding Playground link, the Playground doesn't show these errors. What's going on?It turns out that, despite our specifying
@noLib: true
in the snippet, when we compile with twoslash, we're "accidentally" ending up with@types/node
in the compilation, because it's in our ownnode_modules
directory. This explains why we don't see the errors in the Playground: there, we use the "in-memory filesystem" provided bycreateSystem
intypescript-vfs
, but during the Gatsby build, we use thecreateFSBackedSystem
, who delegates to the real filesystem, so that when TS goes looking fornode_modules/@types
per the defaulttypeRoots
behavior, it will discover the one from our own project.Specifically, our errors are because
@types/node
contains a type definition forArray<T>
, but in our snippet, we try to defineArray<Type>
with a differently-named type parameter. Therefore, we could work around the error by simply changingType
toT
. However, since our intention with@noLib: true
was thatArray
would have no other declarations, the current behavior seems wrong, and therefore we should find a better solution.It actually gets a little weirder, since
@types/node
contains a triple-slash reference to the default ES2020 lib, so now we're really getting the lib who we asked not to get 🙂 furthermore, it's the ES2020 lib, even though ourtarget
is set to ES2016 in the twoslash default options.I discovered this behavior by stepping through the twoslash logic in the debugger and inspecting things like:
What docs actually change with this change
I did diffs of the Gatsby output
public
directory to get a concrete answer to this.I found only 2 places where losing third-party types results in a "decrease in correctness," which we can fix by specifying
@types
in the snippet. Then there's the aforementioned change to use theconsole
types from the DOM lib (either an "increase in correctness" or a no-op 🙂).Like mentioned above, our "accidental" inclusion of
@types/node
resulted in the transitive inclusion of the ES2020 lib. When we lose this, we fall back to our explicitly-configuredtarget
of ES2016. This would be a problem for snippets who use APIs from those newer ES versions; seems like there's only 1 place where this happens, though: in "Basics", when we usetoLocaleLowerCase
:Since we were getting ES2020 without knowing it anyway, we might as well make it our default
target
.