Closed boneskull closed 9 months ago
We don't have to use JSDoc @typedefs to migrate away, just need to "runtime" re-export the types file from the entrypoint, but that does require generating type definition files in a prepublish build step, which these packages do not do yet.
@mhofman I don't follow. Say we wanted to expose PackageNamingKit
from types.js
in @endo/component-mapper
. How would we do that, exactly?
@mhofman I don't follow. Say we wanted to expose
PackageNamingKit
fromtypes.js
in@endo/component-mapper
. How would we do that, exactly?
types.d.ts
which seems to only be a "types" copy of runtime exports present in index.js
.exports.js
with only export {};
as contentexports.d.ts
, with the following as content: export { PackageNamingKit } from './src/types.js';
export * from './exports.js';
in index.js
"types"
field from package.json
prepublish
step to generate .d.ts
files from .js
files like some other packages are already doing@mhofman I'll try that.
But it begs the question: why use a types.js
anyway, when we could just rewrite it as types.ts
using the convenient syntax? To me, a .js
file containing only types seems like it should be a .ts
file. Is there a practical/technical concern? Or a preference / philosophical one?
In this case, exports.js
and export.d.ts
would just become exports.ts
, and the .d.ts
would be generated in the prepack
lifecycle script, then removed after.
(btw, compartment-mapper
already compiles declarations in its prepack
script then blasts them in its postpack
script)
@mhofman By "I'll try that", I mean I will try that in another forthcoming PR. Care to approve?
FWIW, I just tried this in compartment-mapper
, and it seems to work fine to:
types
anything from package.json
src/exports.ts
which contains only export type * from './types.js'
export * from './src/exports.js'
in the root index.js
Mind you, within a node16/nodenext
repo consuming @endo/compartment-mapper
, TS finds no types whatsoever unless the .d.ts
files have been generated. e.g. If I link compartment-mapper
into my own project, I have to run its prepack
script to have type information. I don't think this is unusual behavior--though it is awkward for my use case.
In this case,
exports.js
andexport.d.ts
would just becomeexports.ts
, and the.d.ts
would be generated in theprepack
lifecycle script, then removed after.
Because that requires the generation of the output not just for publish but also for authoring inside the repo, which I am against.
TS finds no types whatsoever unless the
.d.ts
files have been generated. e.g. If I linkcompartment-mapper
into my own project, I have to run itsprepack
script to have type information. I don't think this is unusual behavior--though it is awkward for my use case.
That is exactly the behavior I find unacceptable. There should be no generation step needed when working within the repo (which effectively also uses links through yarn workspace)
@mhofman
Because that requires the generation of the output not just for publish but also for authoring inside the repo, which I am against.
It doesn't.
To be clear, the changes to package.json
and the addition of exports.ts
does not change how @endo/compartment-mapper
interfaces with the rest of the workspaces in the project
That is exactly the behavior I find unacceptable. There should be no generation step needed when working within the repo (which effectively also uses links through yarn workspace)
There isn't!
It's only when working outside of the repo. By my own project I mean "a package somewhere in the lavamoat monorepo which links @endo/compartment-mapper
."
I'm confident this all works the way you think it should.
if there is an export * from './src/export.js'
in index.js
that means an export.js
must exist or else the package will fail to load at runtime. If that file is generated, it means a generation step is required. Note this is not a concern to make types work, but a concern to keep runtime imports of the package working when the package is imported by other packages in the same repo.
@mhofman Ah, you're right. Anyway: the code in question doesn't exist. I am trying to get this PR merged, though. ;)
cc @naugtur @kriskowal
Closes #1802
Description
TypeScript's
node16
andnodenext
module resolution algorithms attempt to adhere more closely to the behavior of theexports
field. To that end, if anexports
field is present in a package, thetypes
field is ignored; instead, atypes
conditional export is expected.Without this change, any consumer using one of these newer--officially recommended--module resolution algorithms will be unable to load any
types.d.ts
file from any package. We retain thetypes
field because of backwards compatibility with the legacynode
module resolution algorithm.Read more about how I came to this conclusion
Should the
types
field be used at all?It depends. While this file is not truly needed for type support if type declarations are shipped alongside each
.js
source file, it's awkward to export any types created via@typedef
without it.For instance, say we have a function
foo(opts: FooOpts)
.foo
lives ina.js
, andFooOpts
is a@typedef
inb.js
. The parameteropts
is tagged as@param {import('./b.js').FooOpts} opts
, as it should. Entry pointindex.js
re-exportsfoo()
froma.js
. However, the definition forFooOpts
will not be available to consumers--which will cause compilation errors. We must, then, re-export this type from the entry point. To avoid this, we have precious few tools at our disposal:FooOpts
inindex.js
via@typedef
, which will export theFooOpts
type. IfFooOpts
is generic, this becomes more of a headache, as we have to copy the generics from the original. This is a hazard, because the generics could become out-of-sync..d.ts
(it could also be a.ts
, which we can assume will generate the associated.d.ts
; this doesn't matter for our purposes) which re-exportsFooOpts
fromb.js
. This file will be the titulartypes.d.ts
.The question we need to ask, then, is "do we need to export any
@typedef
s?" If the answer is yes, then atypes.d.ts
makes sense.I didn't actually check to see if
types.d.ts
is appropriate in all of these packages; I just fixed the problem I was encountering. For@endo/compartment-mapper
, some work I'm doing will mean re-exporting some@typedef
s, even if that's not happening currently. Furthermore, I ignored the packages with"types": null
, since I don't understand what that's supposed to mean.Testing Considerations
I did not add any tests, as the test needs to happen on the result of
npm pack
, which is more of a fixture than I wanted to build out. However, I am working on a tool which will (hopefully soon) automatically check for problems like this. Once that's ready, I can open a new PR to add the checks and you can see what you think.