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

fix: add "types" field to conditional exports #470

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

TomAFrench
Copy link
Contributor

I'm experiencing a lot of issues when trying to resolve the types

src/fft/single_fft.ts:1:26 - error TS7016: Could not find a declaration file for module 'threads'. '/home/tom/Programming/turbo-prover-js/node_modules/threads/index.mjs' implicitly has an 'any' type.
  There are types at '/home/tom/Programming/turbo-prover-js/node_modules/threads/dist/index.d.ts', but this result could not be resolved when respecting package.json "exports". The 'threads' library may need to update its package.json or typings.

1 import { Transfer } from 'threads';
                           ~~~~~~~~~

src/pippenger/single_pippenger.ts:3:26 - error TS7016: Could not find a declaration file for module 'threads'. '/home/tom/Programming/turbo-prover-js/node_modules/threads/index.mjs' implicitly has an 'any' type.
  There are types at '/home/tom/Programming/turbo-prover-js/node_modules/threads/dist/index.d.ts', but this result could not be resolved when respecting package.json "exports". The 'threads' library may need to update its package.json or typings.

3 import { Transfer } from 'threads';
                           ~~~~~~~~~

src/wasm/worker.ts:2:37 - error TS7016: Could not find a declaration file for module 'threads/observable'. '/home/tom/Programming/turbo-prover-js/node_modules/threads/observable.mjs' implicitly has an 'any' type.
  Try `npm i --save-dev @types/threads` if it exists or add a new declaration (.d.ts) file containing `declare module 'threads/observable';`

2 import { Subject, Observable } from 'threads/observable';
                                      ~~~~~~~~~~~~~~~~~~~~

src/wasm/worker.ts:3:34 - error TS7016: Could not find a declaration file for module 'threads/worker'. '/home/tom/Programming/turbo-prover-js/node_modules/threads/worker.mjs' implicitly has an 'any' type.
  Try `npm i --save-dev @types/threads` if it exists or add a new declaration (.d.ts) file containing `declare module 'threads/worker';`

3 import { expose, Transfer } from 'threads/worker';
                                   ~~~~~~~~~~~~~~~~

src/wasm/worker_factory.ts:2:39 - error TS7016: Could not find a declaration file for module 'threads'. '/home/tom/Programming/turbo-prover-js/node_modules/threads/index.mjs' implicitly has an 'any' type.
  There are types at '/home/tom/Programming/turbo-prover-js/node_modules/threads/dist/index.d.ts', but this result could not be resolved when respecting package.json "exports". The 'threads' library may need to update its package.json or typings.

2 import { spawn, Thread, Worker } from 'threads';

The last error explicitly calls out that changes need to be made to the threads.js package.json. I've then added a "types" field for each of the conditional exports.

@TomAFrench
Copy link
Contributor Author

Related to #462

@tien
Copy link

tien commented May 21, 2023

default needs to come last else it will throw an error

Copy link
Contributor

@Altrue Altrue left a comment

Choose a reason for hiding this comment

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

Since it's been more than a month now, someone had to do it to stop the PR from stagnating :)

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@CMCDragonkai
Copy link

Yea the types in this package doesn't work. In my vscode it says:

Could not find a declaration file for module 'threads'. '/home/cmcdragonkai/Projects/js-workers/node_modules/threads/index.mjs' implicitly has an 'any' type.
  There are types at '/home/cmcdragonkai/Projects/js-workers/node_modules/threads/dist/index.d.ts', but this result could not be resolved when respecting package.json "exports". The 'threads' library may need to update its package.json or typings.ts(7016)

This with native ESM support.

When I was using CJS, I could directly import the type files, hacking around the dist. But with ESM, I think you need to specify the types key properly above require, and default... etc.

@CMCDragonkai
Copy link

These are the types I care about:

import type { ModuleThread } from 'threads';
import type { ModuleMethods } from 'threads/dist/types/master';
import type { QueuedTask } from 'threads/dist/master/pool-types';

They used to work when I was using CJS. Now I'm transitioning to ESM, and the last 2 imports no longer work.

Co-authored-by: Altrue <9656008+Altrue@users.noreply.github.com>
@TomAFrench
Copy link
Contributor Author

TomAFrench commented Aug 13, 2023

@Altrue @tien Ah, thank you. I must admit I didn't test that as a required ordering to fields in JSON doesn't really make sense.

I can't quite remember why I was running into this issue anymore (most likely a side project I'm not particularly active in Ah, this was for work but we've gone down a different path) so this isn't a huge priority to me anymore and @andywer doesn't seem to be actively merging PRs currently.

@CMCDragonkai
Copy link

CMCDragonkai commented Sep 26, 2023

@andywer are you still working on this? Without this PR I think ESM downstream projects can't use this.

And I actually care about these types: #470 (comment)

@Anton-Plagemann
Copy link

Hi @andywer,
I've ran into the same issue, fixed them with the changes from this PR (using patch-package).
Would be great if you could merge it 🙏

@andywer
Copy link
Owner

andywer commented Jun 19, 2024

Hi @Anton-Plagemann & @TomAFrench, sorry for the unresponsiveness!

Will merge right away.

@andywer andywer merged commit 5f9a15f into andywer:master Jun 19, 2024
@andywer andywer mentioned this pull request Jun 19, 2024
@andywer
Copy link
Owner

andywer commented Jun 19, 2024

Tests are broken, unfortunately. Updated some dependencies on https://github.com/andywer/threads.js/tree/chore/fix-ci, but still have an issue with a worker in a browser. The statically deployed worker script doesn't exist anymore… 😕

@oxcl
Copy link

oxcl commented Jun 23, 2024

@andywer hi.
is there anything that needs to be done to speed up the process so that we can get a new release with this fix in npm?

@TomAFrench TomAFrench deleted the add-types-to-package.json branch June 23, 2024 14:03
@andywer
Copy link
Owner

andywer commented Jun 23, 2024

@oxcl If someone could fix the build, that would be amazing. I could also build and publish despite the failing test, but we should definitely fix this in the mid-term.

@oxcl
Copy link

oxcl commented Jun 24, 2024

@oxcl If someone could fix the build, that would be amazing. I could also build and publish despite the failing test, but we should definitely fix this in the mid-term.

@andywer I just ran npm audit fix on the project and it seems like the problem went away.
both npm run build and npm run test run successfully. should i submit a PR?

@andywer
Copy link
Owner

andywer commented Jun 24, 2024

@oxcl Very hard to believe. The worker script deployment doesn't exist anymore:

const hello = await spawn<HelloWorker>(new Worker("https://infallible-turing-115958.netlify.com/hello-worker.js"))

Unfortunately, I also don't know if I have the code anywhere else, but I think it was just the transpiled and bundled hello-world.ts worker IIRC. Maybe we don't even need bundling anymore and can just use ES module imports from esm.sh or so.

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.

7 participants