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 open package #492

Closed
wants to merge 2 commits into from
Closed

add open package #492

wants to merge 2 commits into from

Conversation

pomdtr
Copy link

@pomdtr pomdtr commented Aug 4, 2022

Fixes #491

  • Tests pass
  • Appropriate changes to README are included in PR
  • Types updated

@google-cla
Copy link

google-cla bot commented Aug 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@@ -36,7 +36,7 @@ npm i -g zx
## Goods

[$](#command-) · [cd()](#cd) · [fetch()](#fetch) · [question()](#question) · [sleep()](#sleep) · [echo()](#echo) · [stdin()](#stdin) · [within()](#within) ·
[chalk](#chalk-package) · [fs](#fs-package) · [os](#os-package) · [path](#path-package) · [glob](#globby-package) · [yaml](#yaml-package) · [minimist](#minimist-package) · [which](#which-package) ·
[chalk](#chalk-package) · [fs](#fs-package) · [opn](#open-package) · [os](#os-package) · [path](#path-package) · [glob](#globby-package) · [yaml](#yaml-package) · [minimist](#minimist-package) · [which](#which-package) ·
Copy link

Choose a reason for hiding this comment

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

Is there a reason why it's called opn instead of open? I think the later is clearer.

Copy link
Author

Choose a reason for hiding this comment

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

Some weird compilation error. It looks like the open keyword is reserved, so I'm not able to use it.

Copy link

@webNeat webNeat Aug 21, 2022

Choose a reason for hiding this comment

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

You are right, I tried it and had this error

Duplicate identifier 'open'.ts(2300)
lib.dom.d.ts(17754, 18): 'open' was also declared here.

It seems that Typescript considers the global object to have properties of window in the browser, which has an open method.

This also explains why fetch is not declared in the globals.ts file, because it's already part of window (and was also added to node global context).

I don't see the "DOM" lib on tsconfig.json so I am not sure where is it coming from ...

I found a related issue on Typescript microsoft/TypeScript#43990

Copy link
Collaborator

Choose a reason for hiding this comment

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

From node-fetch =(

Copy link

Choose a reason for hiding this comment

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

Ah, I see. Do we really need node-fetch now that node has fetch by default?

Copy link
Collaborator

Choose a reason for hiding this comment

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

node has fetch by default

We can! But right now zx support >= node16. So until the next major zx we can't change it.

@antonmedv
Copy link
Collaborator

Let's not add this to zx. Zx already really big. I'm thinking of adding something like

await npm('open').open(...)

@pomdtr
Copy link
Author

pomdtr commented Aug 21, 2022

Let's not add this to zx. Zx already really big. I'm thinking of adding something like

await npm('open').open(...)

Interesting, it reminds me of the npm util from scriptkit: https://www.scriptkit.com/docs/npm-packages

@webNeat
Copy link

webNeat commented Aug 21, 2022

await npm('open').open(...)

So, if I understand correctly, it will be possible to use npm dependencies without having a package.json and node_modules?

If that's the case, it will be awesome!

@pomdtr
Copy link
Author

pomdtr commented Aug 21, 2022

I found this library which tackle the "import at runtime" issue: https://www.npmjs.com/package/live-plugin-manager

@pomdtr
Copy link
Author

pomdtr commented Aug 27, 2022

closed in favour of #498

@pomdtr pomdtr closed this Aug 27, 2022
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.

Add open package
3 participants