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

lib: add require('🦕') #35478

Closed
wants to merge 2 commits into from
Closed

lib: add require('🦕') #35478

wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

lib: add require('🦕') 492291a

Provide an easy upgrade path to Node.js users that want to make the
switch to Deno.

To ease the transition, the 🦕 module will download the deno runtime
on demand.

Comes included with tests, documentation and minor tweaks to support
non-ASCII filenames.

Refs: https://deno.land/


doc: move bnoordhuis to emiritus 25d31a7

It's been ten years and it's time for a change. September 30 was my
last day at IBM and my last day as a full-time Node.js maintainer.

I feel Deno is the future and that's why I'm joining that scrappy
startup, Deno Land Inc., to make that future happen.

Stay safe and I hope to see you all around! 👋

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 3, 2020
@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 3, 2020
Comment on lines +452 to +453
* [bnoordhuis](https://github.com/bnoordhuis) -
**Ben Noordhuis** <info@bnoordhuis.nl>
Copy link
Member

Choose a reason for hiding this comment

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

README changes/commit are unrelated and included in another PR?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I guess they're not "unrelated" exactly...

doc/api/🦕.md Outdated
<!-- source_link=lib/🦕.js -->

The `deno` module executes the [deno][] runtime, downloading it on demand
if necessary. Currently only supported on Linux, macOS and Windows.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if necessary. Currently only supported on Linux, macOS and Windows.
if necessary. Currently only supported on Linux, macOS, and Windows.

doc/api/🦕.md Outdated
* `args` {string[]} List of string arguments.
* `options` Options passed to [`child_process.spawn()`][]. The optional `exe`
option is the path to the `deno` binary.
* `callback` {Function} called `deno` terminates.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `callback` {Function} called `deno` terminates.
* `callback` {Function} Called `deno` terminates.

doc/api/🦕.md Outdated
[deno]: https://deno.land/
[`child_process.spawn()`]: #child_process_child_process_spawn_command_args_options
Copy link
Member

Choose a reason for hiding this comment

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

Linter will want these two lines switched with each other.

@Trott
Copy link
Member

Trott commented Oct 3, 2020

Seems like there's nothing keeping this from being a userland module, other than maybe the module name?

@Trott
Copy link
Member

Trott commented Oct 3, 2020

Come clean, Ben. You're only opening this PR so you can get a Hacktoberfest t-shirt, right?

Copy link
Member

@himself65 himself65 left a comment

Choose a reason for hiding this comment

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

join Deno? Really?🤔🤔🤔

Provide an easy upgrade path to Node.js users that want to make the
switch to Deno.

To ease the transition, the 🦕 module will download the deno runtime
on demand.

Comes included with tests, documentation and minor tweaks to support
non-ASCII filenames.

Refs: https://deno.land/
It's been ten years and it's time for a change. September 30 was my
last day at IBM and my last day as a full-time Node.js maintainer.

I feel Deno is the future and that's why I'm joining that scrappy
startup, Deno Land Inc., to make that future happen.
@bnoordhuis
Copy link
Member Author

Feedback incorporated, PTAL.

Seems like there's nothing keeping this from being a userland module, other than maybe the module name?

Exactly. I don't think npm allows emoji module names.

Come clean, Ben. You're only opening this PR so you can get a Hacktoberfest t-shirt, right?

Aw, was it that transparent? I deliberately left out "amazing project"!

@gengjiawen
Copy link
Member

This made my day 😃

if (typeof cb !== 'function') cb = () => {};

const defaults = {
download: './' + EXE,
Copy link

@SukkaW SukkaW Oct 3, 2020

Choose a reason for hiding this comment

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

Really? Download deno binary directly to cwd?

Copy link
Member

Choose a reason for hiding this comment

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

make you days happy😂

@devsnek
Copy link
Member

devsnek commented Oct 3, 2020

this is too spooky, even for october

@mmarchini
Copy link
Contributor

The real question though: was this tested with import '🦕' and node -r '🦕'?


const EXE = (process.platform === 'win32') ? 'deno.exe' : 'deno';
const TRIPLET = triplet(process.arch, process.platform);
const VERSION = '1.4.3';
Copy link
Member

Choose a reason for hiding this comment

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

Could we make version dynamic?


function download(filename, cb) {
const url =
'https://github.com/denoland/deno' +
Copy link
Member

@vdeturckheim vdeturckheim Oct 3, 2020

Choose a reason for hiding this comment

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

Why enforce https? Web standards allow the choice of it when importing modules.

Also, could this support mirrors too?

<!-- source_link=lib/🦕.js -->

The `deno` module executes the [deno][] runtime, downloading it on demand
if necessary. Currently only supported on Linux, macOS, and Windows.
Copy link
Member

Choose a reason for hiding this comment

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

Will it work on Alpine too or only glibc linuxes?


<!--introduced_in=REPLACEME-->

> Stability: 1 - Experimental
Copy link
Member

Choose a reason for hiding this comment

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

Is there is clear path out of experimental for this?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Generally a hard -1 on this in core. This should be a userland module.

@nodejs nodejs deleted a comment Oct 3, 2020
@Trott Trott closed this Oct 5, 2020
@jasnell
Copy link
Member

jasnell commented Oct 27, 2020

@bnoordhuis ... This PR included a commit to move you to emeritus status. Did you still want to do that?

@sayantan300
Copy link

lmfao? cant stop laughing... it would be a great thing if this didnt downloaded exe stuffs and just could port deno to node lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.