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 modern javascript #432

Closed
antialias opened this issue Nov 28, 2020 · 2 comments
Closed

support modern javascript #432

antialias opened this issue Nov 28, 2020 · 2 comments
Assignees

Comments

@antialias
Copy link
Contributor

antialias commented Nov 28, 2020

the situation

According to the .nvmrc file in this project, javascript syntax must be limited to what is supported by Node 10.18.1. As I write this, Node LTS is currently at v.14.15.1. Since node 14 supports some nice features that this (and any) javascript project will benefit from, I would like to propose that we enable this project to take advantage of modern javascript syntax and all that it has to offer.

Specifically, I would appreciate being able to use the following features that are not natively supported by node 10:

motivations

motivation 1: While these features aren't specific to this resume-cli's purpose of creating a resume through the command line from a .json file, the clarity of this project's codebase can be significantly improved with the use of these features, thereby making it more accessible to future (and existing) contributors.

motivation 2: I forked resume-cli and addressed nearly every relevant outstanding issue (as of April 2020) and would like to begin posting PRs for review that that contain small pieces of code from my fork without having to downgrade the syntax so an older version of node can read it.

what to do

There are two ways we could go about using modern javascript in this project:

native support from node

  • PR feat!: using node14 with type:module #434
  • how to get there
    • set node v14 in .nvmrc
    • set "type": "module" in package.json
    • codeshift all instances of require / modules.exports to import / export
  • pros
    • no extra configuration required for linters and test runners
  • cons
    • need to ensure that anyone that pulls in this change runs nvm install before running the code
    • may break projects that use resume-cli as a dependency and earlier versions of node. According to npm, resume-cli has 11 dependents at the moment.

transpile down to an older version of node

  • PR: build(babel): transpiling with babel so we can use modern javascript #433
  • how to get there
    • add babel as a dev dependency
    • pass source in lib through babel on the prepare lifecycle npm event.
  • pros
    • project remains agnostic about which version of node is used to run it
    • most modern js projects use babel, so this is a proven strategy
    • allows quick adoption of future javascript features and syntax ahead of official support from node
  • cons
    • additional build step
    • more install dependencies for developers
    • requires a little extra configuration for integration with mocha (jest supports babel natively if we decide to use it in the future)

My recommendation - both

While both options presented address this issue, they are not mutually exclusive. We really should be using the latest node LTS version anyway, and it would be nice to always have the option to use the latest javscript syntax offered through babel without having to wait for it to be added to node LTS.

@rbardini
Copy link
Contributor

I agree with using the latest Node.js LTS, although I'd prefer moving to TypeScript instead. Besides compiling to different targets, like Babel does, it brings many other benefits and might even be simpler to integrate (as long as we allowJs during the transition).

@antialias
Copy link
Contributor Author

antialias commented Nov 29, 2020

The Babel PR adds a build script and allows for using module syntax and optional chaining, which was my only objective.

Adding typescript would build upon the changes that the Babel PR introduces, so my recommendation is to merge the Babel PR first, then you can set up a PR for typescript.

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

3 participants