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

import @agoric/orchestration fails to find types.js #10086

Closed
dckc opened this issue Sep 13, 2024 · 6 comments · Fixed by #10202
Closed

import @agoric/orchestration fails to find types.js #10086

dckc opened this issue Sep 13, 2024 · 6 comments · Fixed by #10202
Assignees
Labels
bug Something isn't working

Comments

@dckc
Copy link
Member

dckc commented Sep 13, 2024

Describe the bug

A clear and concise description of what the bug is.

To Reproduce

Steps to reproduce the behavior:

  1. put the code below in packages/builders/scripts/orch-imp.js
  2. run node scripts/orch-imp.js
  3. Error
import * as orch from '@agoric/orchestration';

console.log(orch);
$ cd agoric-sdk/packages/builders
$ node scripts/orch-imp.js 
node:internal/errors:496
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/connolly/projects/agoric-sdk/packages/orchestration/src/types.js' imported from /home/connolly/projects/agoric-sdk/packages/orchestration/index.js
    at new NodeError (node:internal/errors:405:5)
    at finalizeResolution (node:internal/modules/esm/resolve:327:11)
    at moduleResolve (node:internal/modules/esm/resolve:980:10)
    at defaultResolve (node:internal/modules/esm/resolve:1193:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:404:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:373:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:250:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:76:39)
    at link (node:internal/modules/esm/module_job:75:36) {
  url: 'file:///home/connolly/projects/agoric-sdk/packages/orchestration/src/types.js',
  code: 'ERR_MODULE_NOT_FOUND'
}

Expected behavior

No error.

Platform Environment

  • what OS are you using? what version of Node.js?

Node.js v18.20.1

  • is there anything special/unusual about your platform?
  • what version of the Agoric-SDK are you using? (run git describe --tags --always)

66e4cc0 (actually a few commits from that version of master: b38b7f3 )

Additional context

@mhofman asked about this in #10057 (comment)

@dckc dckc added the bug Something isn't working label Sep 13, 2024
@turadg
Copy link
Member

turadg commented Sep 13, 2024

Simpler repro,

node --input-type=module -e "import '@agoric/orchestration'"

Reasons not to change this:

  • The fix, using .d.ts, skips typechecking
  • Imports work fine from NPM installations (this affects only repo peer packages)
  • Even barrel imports work fine from a .ts file
  • Repo peer packages can still use deep imports
  • This has not been a problem yet in the course of building orchestration and testing it in packages/boot and multichain-testing

@dckc
Copy link
Member Author

dckc commented Sep 13, 2024

Imports work fine from NPM installations (this affects only repo peer packages)

How does that work?

I'm struggling to confirm, fwiw.

$ mkdir /tmp/fun1
$ cd /tmp/fun1
$ yarn add @agoric/orchestration@dev

$ node --input-type=module -e "import '@agoric/orchestration'"
node:internal/modules/esm/resolve:265
    throw new ERR_MODULE_NOT_FOUND(
          ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module ...

@turadg
Copy link
Member

turadg commented Sep 13, 2024

How does that work?

I misremembered that it uses a dist folder like @agoric/cosmic-proto does.

It already has a prepack that does tsc build. Top-level imports with NPM installations can work if we make it output to dist and set the entrypoints to that… but then we'd have to build dist as part of our monorepo workflow, which we've avoided.

So I don't have a good solution. There's a trade-off to be made.

A reasonable stop-gap is that we remove types.js from the index and export it at /types.js.

@mhofman
Copy link
Member

mhofman commented Sep 13, 2024

I really don't think we should compromise on the public shape, which includes importing types from the entry point.

I'm wondering if there's a possible solution for this type check issue: another level of indirection. Have index.js import from ./types.js which is defined as a manual empty .js file and a .d.ts file which simply does export * from './src/types.js' which is actually written as types.ts

@turadg
Copy link
Member

turadg commented Sep 13, 2024

Good idea. I suppose a manual index.d.ts would also work, but risk getting out of sync with index.js. I like empty types.js for indirection because it will always be empty.

@mhofman
Copy link
Member

mhofman commented Sep 13, 2024

To clarify however, the output would be alongside the src, and not in a dist folder, right ?

@LuqiPan LuqiPan assigned 0xpatrickdev and unassigned LuqiPan Sep 23, 2024
0xpatrickdev added a commit that referenced this issue Sep 25, 2024
- add types.js indirection layer to fix ERR_MODULE_NOT_FOUND error.
- maintains public API shape for intra-monorepo and npm installs.
- closes #10086
0xpatrickdev added a commit that referenced this issue Sep 25, 2024
- add types.js indirection layer to fix ERR_MODULE_NOT_FOUND error.
- maintains public API shape for intra-monorepo and npm installs.
- closes #10086
0xpatrickdev added a commit that referenced this issue Sep 25, 2024
- add types.js indirection layer to fix ERR_MODULE_NOT_FOUND error.
- maintains public API shape for intra-monorepo and npm installs.
- refs #10086
0xpatrickdev added a commit that referenced this issue Sep 25, 2024
- add types.js indirection layer to fix ERR_MODULE_NOT_FOUND error.
- maintains public API shape for intra-monorepo and npm installs.
- refs #10086
mergify bot added a commit that referenced this issue Sep 25, 2024
refs: #10086

## Description

- add types.js indirection layer to fix ERR_MODULE_NOT_FOUND error.
- maintains public API shape for intra-monorepo and npm installs.

### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
n/a

### Testing Considerations
Includes a test in `packages/builders` that ensures `@agoric/orchestration` exports are reachable from a js file.

### Upgrade Considerations
Library code for NPM Orch release
mergify bot added a commit that referenced this issue Sep 28, 2024
refs: #10086

## Description

Document the practice established in #10149 into a Best Practice.

Adopt it in most packages. I only left these `types.d.ts`
-  swing-store because the file is only re-exports, nothing to typecheck
- smart-wallet because there's no entrypoint for the package


### Security Considerations
none

### Scaling Considerations
none

### Documentation Considerations
per se

### Testing Considerations
#10149 (comment) asked for more testing. This PR was originally intended to close the issue but the regression test is complicated enough and this PR has gotten big enough that I think it should land on its own.

### Upgrade Considerations
n/a
mergify bot added a commit that referenced this issue Oct 1, 2024
refs: #10086

## Description

Brings all remaining packages into compliance with the best practice documented in #10158

Also has some other cleanup, separated by commit

### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
none

### Testing Considerations
none

### Upgrade Considerations
none
@turadg turadg assigned turadg and unassigned 0xpatrickdev Oct 2, 2024
@mergify mergify bot closed this as completed in #10202 Oct 3, 2024
@mergify mergify bot closed this as completed in 5b3e4d0 Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants