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

More runtime compatibility #28

Closed
Airkro opened this issue Sep 13, 2021 · 9 comments
Closed

More runtime compatibility #28

Airkro opened this issue Sep 13, 2021 · 9 comments

Comments

@Airkro
Copy link

Airkro commented Sep 13, 2021

p-* function is useful, thank you for your great work.

Recently I had some trouble when I use them in some non node.js runtime, without bundler.

The error cause by:

p-* -> https://github.com/sindresorhus/aggregate-error -> https://github.com/sindresorhus/clean-stack/blob/main/index.js#L1

There is no os module in non node.js runtime.

So, it's possible to stop using aggregate-error in p-*?

@sindresorhus
Copy link
Owner

The only way I can think of to fix support this is to use an exports map in package.json with a separate browser.js file for browsers that do not import os. Most users use a bundler for browser-based stuff though, so this is not something I plan to work on, but a good pull request would be welcome.

@sindresorhus
Copy link
Owner

So, it's possible to stop using aggregate-error in p-*?

Yes, when my packages can target Node.js 16 as then there's a native AggregateError.

@sindresorhus sindresorhus transferred this issue from sindresorhus/promise-fun Sep 13, 2021
@Airkro
Copy link
Author

Airkro commented Sep 13, 2021

I understand why aggregate-error matters now, but I don't think clean-stack has to be a part of it, native AggregateError doesn't have that features (I guess).

Remove clean-stack or cleanStack.options.pretty could be the way, not that pretty but more compatibility?

@sindresorhus
Copy link
Owner

sindresorhus commented Sep 13, 2021

I'm not interested in making breaking changes to aggregate-error. It works differently from the native error type as it was made years before the native error type was introduced.

I have outlined the solution in #28 (comment)

@silverwind
Copy link
Contributor

silverwind commented May 23, 2022

I too had to polyfill node os to get p-all to work in browsers which seems a bit silly.

Imho, aggregate-error should drop its dependency on clean-stack as the stack cleaning performed there seems rather opinionated.

timrogers added a commit to timrogers/obsidian-share-as-gist that referenced this issue Jul 5, 2022
We use Octokit to interact with the GitHub API. This switches
from the `octokit` package to the more specific `@octokit/rest`,
which avoids a dependency on `@octokit/webhooks`.

Avoiding this dependency is good because it in turns depends on
`clean-stack`, which doesn't play nicely in all JS runtimes - see
sindresorhus/clean-stack#28.

This cleans up at least the immediate cause of #25.
timrogers added a commit to timrogers/obsidian-share-as-gist that referenced this issue Jul 5, 2022
We use Octokit to interact with the GitHub API. This switches
from the `octokit` package to the more specific `@octokit/rest`,
which avoids a dependency on `@octokit/webhooks`.

Avoiding this dependency is good because it in turns depends on
`clean-stack`, which doesn't play nicely in all JS runtimes - see
sindresorhus/clean-stack#28.

This cleans up at least the immediate cause of #25.
@fregante
Copy link

fregante commented Jul 16, 2022

I think solving this issue for the browser is easy:

- import os from 'os';
+ import os from './os';

Then have

// os.js
export {default} from 'os'

// os-browser.js
export default {homedir: ''};

The only problem is that bundler compatibility is spotty and I'm not sure about the modern equivalent of the browser field of package.json.

Edit: is it exports.default? https://github.com/sindresorhus/p-state/blob/6cbae7cd3acd2a6c2ca5a5975fbe1569a65b71f9/package.json#L14-L17

I think I've seen this pattern being used with the exports field somewhere in Sindre’s repo. Anyone remembers how to do it and whether the major bundlers support it? Worst case scenario those bundlers keep using os instead of {homedir: ''}

Yes, when my packages can target Node.js 16 as then there's a native AggregateError.

Info: Node 14 EOL is May 2023

@silverwind
Copy link
Contributor

silverwind commented Sep 27, 2022

This new imports usage seems to break at least Jest (in ESM mode) which apparently does not support such resolution.

    Cannot find module '#home-directory' from 'node_modules/clean-stack/index.js'

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:487:11)
          at async Promise.all (index 1)

@sindresorhus
Copy link
Owner

It's up to Jest to support Node.js features.

@silverwind
Copy link
Contributor

For anyone else ending up here: The Jest issue is tracked in jestjs/jest#12270, there seem to be workarounds available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants