fix(ses): fix types export for newer module resolutions #1840

boneskull · 2023-10-27T21:29:33Z

Much like #1803, this adds types conditional exports where appropriate.

Additionally, I fixed a type in ThirdPartyStaticModuleInterface which I was bumping up against.

mhofman

The changes to package.json is fine. It's necessary given the way the "exports" and "types" fields work.

mhofman · 2023-10-27T22:02:55Z

packages/ses/types.d.ts

@@ -59,7 +59,7 @@ export interface ThirdPartyStaticModuleInterface {
  imports: Array<string>;
  exports: Array<string>;
  execute(
-    proxiedExports: Object,
+    proxiedExports: Record<PropertyKey, any>,


I'll defer the review of this change to @kriskowal

The problem with Object is that you cannot assign to it. You can't assign to object, either.

And I defer to the submitter since we can rely on @boneskull to answer support questions on Matrix going forward :-). I have no idea. Did not know PropertyKey was a TS built-in.

Yep! It's string | number | symbol from the es5 internal lib.

Is this object parameter meant to be mutated? That seem contrary to our programming model of hardening what is passed.

Summoning @naugtur to rule on the ESM<->CJS namespace bindings behavior for symbols. My perception is that they would appear on namespace.default[symbol] but not be replicated as namespace[symbol].

So the value of exports as visible from inside of CJS modules is really best described as any because you can do module.exports = function(){} etc.
That exports reference then gets assigned to namespace.default.
The named exports get populated by iterating over keys on that default export. The key (pun intended) question is which keys are taken into account and I'm using Object.keys there which skips over symbols.
I'm also pretty sure the cjs-module-analyzer is not going to pick up symbols. (I'm trying to prove myself wrong with a testcase now. [edit] Confirmed - there's no way the static analysis the lexer there is capable of is finding symbol references.)

In conclusion, I support Record<string, any> and the comment about the ESM/CJS difference seems invalid.

@naugtur Understood. This begs the question, though, should Object.keys() be used there, and not, say, Object.getOwnPropertyDescriptors()?

I've updated the comment.

I'm not sure if it's possible for whatever Object.keys would skip to be detected as named exports by the lexer and only the pre-defined top-level fields will be able to populate from cjs to esm. But I can't say you're not right pointing that out.

One case I can think of is having a getter for lazy loading. I wonder if copying a getter over would even work as intended.

kriskowal · 2023-10-30T18:58:34Z

packages/ses/package.json

@@ -36,11 +36,13 @@
  "exports": {
    ".": {
      "import": "./index.js",
-      "require": "./dist/ses.cjs"
+      "require": "./dist/ses.cjs",
+      "types": "./types.d.ts"


Can you add a line to package.json.md explaining this?

@kriskowal Done. Also fixed another path in there that was wrong.

boneskull · 2023-10-30T19:28:01Z

@kriskowal Looks like CI needs to be burped

boneskull · 2023-10-31T21:05:29Z

Please note that #1803 is the same sort of thing but for @endo/compartment-mapper (and is awaiting approvals)

Much like endojs#1803, this adds `types` conditional exports where appropriate.

The `proxiedExports` param to the `execute` function cannot be `Object`, which is the literal `Object`. It must be _some_ kind of `Object`, however, a `Record` is suitable. However, the property can be a `PropertyKey` if the exports come from CommonJS and can only be a `string` if the exports come from ESM. Use `string` for now and we can widen the type later.

mhofman approved these changes Oct 27, 2023

View reviewed changes

kriskowal reviewed Oct 30, 2023

View reviewed changes

kriskowal approved these changes Oct 30, 2023

View reviewed changes

boneskull force-pushed the boneskull/fix-type-exports-ses branch from 47dbf9c to 0e819f3 Compare October 30, 2023 19:11

boneskull force-pushed the boneskull/fix-type-exports-ses branch 3 times, most recently from f8424fd to 61db03b Compare October 31, 2023 21:00

boneskull force-pushed the boneskull/fix-type-exports-ses branch from 61db03b to 266aab5 Compare November 6, 2023 22:13

boneskull added 2 commits November 6, 2023 14:19

fix(ses): fix types export for newer module resolutions

9cc3dd5

Much like endojs#1803, this adds `types` conditional exports where appropriate.

boneskull force-pushed the boneskull/fix-type-exports-ses branch from 266aab5 to fe38c40 Compare November 6, 2023 22:19

kriskowal enabled auto-merge November 6, 2023 22:20

kriskowal merged commit b2cb361 into endojs:master Nov 6, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ses): fix types export for newer module resolutions #1840

fix(ses): fix types export for newer module resolutions #1840

boneskull commented Oct 27, 2023

mhofman left a comment

mhofman Oct 27, 2023

boneskull Oct 30, 2023

kriskowal Oct 30, 2023

boneskull Oct 30, 2023

mhofman Oct 30, 2023

kriskowal Oct 30, 2023 •

edited

Loading

naugtur Oct 31, 2023 •

edited

Loading

boneskull Oct 31, 2023

boneskull Oct 31, 2023

naugtur Nov 13, 2023

kriskowal Oct 30, 2023

boneskull Oct 30, 2023

boneskull commented Oct 30, 2023

boneskull commented Oct 31, 2023 •

edited

Loading

fix(ses): fix types export for newer module resolutions #1840

fix(ses): fix types export for newer module resolutions #1840

Conversation

boneskull commented Oct 27, 2023

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

naugtur Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull commented Oct 30, 2023

boneskull commented Oct 31, 2023 • edited Loading

kriskowal Oct 30, 2023 •

edited

Loading

naugtur Oct 31, 2023 •

edited

Loading

boneskull commented Oct 31, 2023 •

edited

Loading