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

Support for Deno #315

Open
quilicicf opened this issue Sep 10, 2020 · 11 comments
Open

Support for Deno #315

quilicicf opened this issue Sep 10, 2020 · 11 comments

Comments

@quilicicf
Copy link

quilicicf commented Sep 10, 2020

Hi guys,

Do you have any plans to add compatibility for deno?

I'd really like to try implementing my CLI scripts with Deno and without a good arguments parser and prompt library it's not very fun.

Yargs is currently moving to Deno, now the only thing that remains is a good prompt library :-)

@jonschlinkert
Copy link
Member

I'd love to add support! I've looked played around with deno a few times. I'd definitely be interested in hearing thoughts on it, but I'd need someone else to take it on (FWIW I'm a single dad and my girls just started back in school, and we're doing distance learning. So my plate is a bit fuller than usual until we get the hang of this).

@quilicicf
Copy link
Author

I have tinkered a bit with Deno and my experience is pretty great so far :-)
The only thing the ecosystem needs to finally be able to compete with NodeJS is modules like enquirer IMO.

My experience so far:

  • code runs a bit slower, probably because of the TS layer but as we can bundle the code in JS it should be mitigated in built deliverables
  • IDE integration is still a bit lacking, on IntelliJ I still have errors on absolute imports for example and need to de-activate/re-activate the plugin from time to time. Not a deal breaker but there's still progress to be done on that front
  • the Deno API is much nicer than Node's
  • the dependency management is nicer than I expected. Leads you directly to the code since you have the link, so it encourages reading the sources which I find pretty nice. Plus it is auto tree-shaken which I think the ecosystem really needed
  • the security will probably become an issue because I'm not aware of anything related to security audits in Deno ATM

Anyway, I'd like to help porting enquirer to Deno but I'm not sure if I can help much on an entirely unknown code-base and with limited time. So no promises but I'll try to have a look.

@quilicicf
Copy link
Author

quilicicf commented Sep 17, 2020

From what I see in the code, these are the NodeJS/external APIs that should be shimed:

API Origin Potential substitute Additional Note State
assert NodeJS Re-implement the two functions used in production file (assert.equals, assert.isTruthy) in pure JS ✔️
events NodeJS https://deno.land/x/events@v1.0.0
ansi-colors npm https://deno.land/std@0.69.0/fmt/colors.ts
readline NodeJS ??? ⚠️ Need help on that one
process.cwd NodeJS Deno.cwd
process.exit NodeJS Deno.exit
process.env NodeJS Deno.env.get, Deno.env.set
process.stdout.columns NodeJS Deno.consoleSize ⚠️ Still unstable at this time, requires --unstable flag
process.stdin NodeJS Deno.sdtin
process.stdin.isTTY NodeJS Deno.isatty(Deno.sdtin.rid)
process.platform NodeJS Deno.build
process.once NodeJS Deno.signal ⚠️ I'm not sure this does the trick, I'd like another pair of eyes there

Research method

Please check I didn't miss anything

  • text search for require( in *.js files (left out examples and tests folders)
  • text search for process. in *.js files (left out examples and tests folders)
  • text search for global in *.js files (left out examples and tests folders)

Migration method

  • Create a shim for NodeJS and another one for Deno that contains a method for each dependency.
  • Add a file index.ts as entrypoint for Deno
  • In both index.* files, import/require the adequate shim
  • Pass it down to dependencies that need it
  • Add a second build phase to build the Deno version
  • Add tests for the shim in both platforms => removes the need to duplicate the whole test suite IMO, it can stay in NodeJS
  • Update README

WDYT?

@quilicicf
Copy link
Author

I have started fiddling around in my fork
I'll keep the previous post updated with my progress.

@quilicicf
Copy link
Author

@jonschlinkert
I think I have tackled the toughest part (migrating the code base to ESM) but it was quite the heavy lifting.
I'd like you to get a chance to review before I go on and add the shim part.

You can review the code here: https://github.com/enquirer/enquirer/compare/master...quilicicf:master_deno?expand=1 (real PR coming in Hacktober hehe...)

Notable parts:

  • add a bundler (parcel currently cause I like it but if you're more comfortable with another I can update this) to handle retro-compatibility with older versions of NodeJS and CommonJS
  • dependency to NodeJS module assert removed from the production code (only used two methods I re-implemented in lib/assert.js)
  • remove use strict because eslint should enforce it. I added an npm script to run eslint (and eslint as a dev dependency), used eslint --fix on the code base and intend to fix the manual things and add the linting as first part of the tests if that's OK with you
  • bump the minimal NodeJS requirement to >=12 because it is the current LTS. The bundled code can be run with NodeJS 8.6 (I did a small smoke test) though.
  • add type: module in package.json to avoid having to rename all the files (and give you a clear diff). I think what should be done ultimately is renaming all files to .mjs except the bundled file (.cjs). Currently, I can't require the bundled file because of the type: module.
  • upgrade mocha, a version 8+ was required to use ESM. I removed the callbacks in the test code where applicable because the new version of mocha understands it must wait for promises to end
  • tweak test/support/index.js so it does not try to update assert but instead uses it to create the method has (has issues with NodeJS refusing to update the assert object)
  • unskip a few tests (they were working, I don't know why they were skipped 🤔 )

Note: I had trouble trying to create a formatter ruleset that does no diff on the code base. I tried to tweak it to match your code style as closely as possible, tell me if there are diffs you want gone.

@quilicicf
Copy link
Author

Current state:

  • the tests run on NodeJS 12.18.3 and NodeJS 14.12.0
  • the bundled code works on both versions above and on NodeJS 8.9.4 too
  • the code base is ready to be adapted for Deno
  • the linter still returns a few errors (35) that can only be fixed manually, I'll tackle that later

@quilicicf
Copy link
Author

quilicicf commented Oct 7, 2020

@jonschlinkert I have opened a PR so you can see my progress (#316 )
I have managed to change the code base so it uses platform shims although I'd like your opinion on it and on the comments I added on things that surprised me before I go on.

It's time consuming so I'd like to get the approach right before investing too much if possible :)

@quilicicf
Copy link
Author

Any chance to get this looked at ? 🙁

@GerbenRampaart
Copy link

I absolutely dread to be one of those 'what's the progress' people on a github issue ... but given this PR is over a year ago and Deno is maturing nicely AND inquirer is a terribly useful module, I'm going to do it anyway, just for this once:

Any progress to solving this?

@quilicicf
Copy link
Author

I stumbled on issues along the way, and don't know the code base enough to make informed decisions about the pain points.
I abandoned, even more so because the PR was becoming crazy huge.

I guess the best way to go forward would be to chunk the changes:

  • switch to ESM imports
  • resolve linter issues
  • then implement Deno support

But without help from the maintainers that's going to prove tough IMO.

I switched to cliffy which is pretty nice and fully developed for Deno. The maintainer is a nice bloke and has taken feedback into account.

@Kreijstal
Copy link

This is kinda useful because deno support will also mean browser support, today I tried importing enquirer on the browser and sadly it has a hard dependency on node's readline, why can't I give my own implementation of readline before the module loads???

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

No branches or pull requests

4 participants