Skip to content
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

Add explicit exports for node #921

Merged
merged 10 commits into from
Nov 27, 2023
Merged

Add explicit exports for node #921

merged 10 commits into from
Nov 27, 2023

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Nov 16, 2023

Related to issue #610 experienced in Protobuf-ES. This adds explicit exports for node into the package.json for all published packages in Connect-ES. In addition, it modifies the path for the top-level import statement to use ESM instead of the proxy.

For more context, check PR #611 in Protobuf-ES as well as the above issue.

Also note that the ideal setting for the node imports condition should be something along the lines of:

  "exports": {
    ".": {
      "node": {
        "import": "./dist/proxy/index.js",
      },
      "module": "./dist/esm/index.js",
      "import": "./dist/esm/index.js",
      "require": "./dist/cjs/index.js"
    }
  },

Since any require statements for Node should find the top-level require condition.

However, doing so causes the attw checks to fail with a Fallback Condition error. So, we have added an additional "require": "./dist/cjs/index.js" inside the node condition to satisfy this check. But, it's unclear whether this is necessary and whether this check is entirely accurate on attw's part.

The claim is that the require condition is being resolved due to a TypeScript bug. The attw docs state that Node's resolution algorithm "requires that resolution stop once a target filename has been tried, whether or not that file could be found". TypeScript, though, will continue to evaluate conditions if a file cannot be found. Hence, why this check fails - attw thinks the require condition is only being resolved because of a bug in TypeScript.

This does not seem to be Node's behavior in practice, though. Node docs clearly state:

Conditions continue to be matched in order as with flat conditions. If a nested condition does not have any mapping it will continue checking the remaining conditions of the parent condition. In this way nested conditions behave analogously to nested JavaScript if statements.

In addition, this has been verified with some local checks. So, the tl;dr here is that the attw Fallback Condition check seems like it may be incorrect, but needs deeper investigation. In the meantime, we are adding this additional node.require condition to satisfy the check until we can definitively say whether it's needed.

packages/connect-express/package.json Outdated Show resolved Hide resolved
.eslintrc.cjs Outdated Show resolved Hide resolved
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

(Please make sure the PR description is up to date)

@smaye81 smaye81 merged commit 033c53c into main Nov 27, 2023
5 checks passed
@smaye81 smaye81 deleted the sayers/node_exports branch November 27, 2023 17:34
smaye81 added a commit to connectrpc/connect-playwright-es that referenced this pull request Dec 5, 2023
This adds explicit exports for node into the package.json for all
published packages in Connect-ES. In addition, it modifies the path for
the top-level import statement to use ESM instead of the proxy.

For additional context, see
connectrpc/connect-es#921
smaye81 added a commit to bufbuild/protobuf-es that referenced this pull request Dec 6, 2023
Fixes #610 

This adds explicit exports for node into the package.json for all
published packages in Protobuf-ES. In addition, it modifies the path for
the top-level import statement to use ESM instead of the proxy.

For additional context, see
connectrpc/connect-es#921
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants