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 support for top-level await #9

Merged
merged 3 commits into from
May 27, 2021

Conversation

duncup
Copy link
Contributor

@duncup duncup commented Oct 9, 2020

This package not support top level await before, but alfy support.

@duncup
Copy link
Contributor Author

duncup commented Oct 19, 2020

@sindresorhus @SamVerschueren
Please review this.

@sindresorhus
Copy link
Collaborator

Can you explain your changes? Why are you now bundling the run-node script?

@SamVerschueren
Copy link
Owner

I don't see why these changes would make it support top level await all of a sudden? It has to do with the underlying Node version that's being used I assume?

Unless Alfred is using one of these environment variables sindresorhus/run-node@9a69ce7, I don't see how these changes would actually make it work?

@duncup
Copy link
Contributor Author

duncup commented Oct 21, 2020

Alfy have support top level await.
sindresorhus/alfy#109
IF i use it, alfy-test has been not work.
And i never see the code in run-node, just copy from the case use in alfy.

@SamVerschueren


There always run it, and isee the pr for alfy how add top level await, and got this script, so i copy it. because i don't think change the run-node is a good idea.
@sindresorhus

@duncup
Copy link
Contributor Author

duncup commented Oct 27, 2020

Anything?

@SamVerschueren
Copy link
Owner

So the thing here is that alfy-test uses a different run-node than alfy. It appears that alfy has a custom run-node script that enables ESM right here.

@sindresorhus would it make sense to create a new alfy-run-node package or something that can be used by alfy-test as well? I guess that would allow us to support ESM here as well.

@sindresorhus
Copy link
Collaborator

I would just copy-paste it here for now. Node.js will eventually support top-level await by default. We can open an issue to revert back to run-node at such time.

@duncup
Copy link
Contributor Author

duncup commented Dec 17, 2020

Anything?

@duncup
Copy link
Contributor Author

duncup commented Feb 20, 2021

Did this project alive?

@SamVerschueren
Copy link
Owner

Sorry, totally missed that you updated your PR. I'll com back to you later today.

@duncup
Copy link
Contributor Author

duncup commented Feb 20, 2021

I know that you guys too busy, so don't be sorry.
But i really need fix this.

Anyway, thank you for you guys make this.

Copy link
Owner

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

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

One small comment.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@SamVerschueren SamVerschueren changed the title fix(*): Make test can use top level await alfy file. Add support for top-level await May 27, 2021
@SamVerschueren SamVerschueren merged commit 516b91f into SamVerschueren:master May 27, 2021
SamVerschueren pushed a commit that referenced this pull request May 27, 2021
@rchl
Copy link

rchl commented Dec 9, 2021

This change is a bit controversial to me.

When having both alfy and alfy-test installed, the alfy-test's .bin/run-node takes over the one included by alfy and breaks things because it tries to use esm which I haven't installed.

The run-node from alfy just runs plain node and it works with top level await if the package has module type and using node 14+.

So if I want to run in plain node without esm then this change breaks things for me. I have to either install esm or only install the production dependencies.

@SamVerschueren
Copy link
Owner

We might want to keep this in line with the run-node.sh from Alfy. https://github.com/sindresorhus/alfy/blob/main/run-node.sh

Alfy moved to ESM entirely in August and thus got rid of the esm flag provided to Node. I think we want to align both of them. PR welcome.

@rchl
Copy link

rchl commented Dec 10, 2021

I guess that would entitle switching this package to be an ES module itself? So a breaking change.

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.

4 participants