Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

[FEATURE/FIX] Ensuring NodeJS 12 Compatibility #315

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sidneys
Copy link

@sidneys sidneys commented Aug 26, 2019

👟 Description & Background

As this Pull Request is not addressing a specific functional issue in code, but rather constitutes a list of fixes to enable the use of this module with a NodeJS 12 context. Currently, referencing text-buffer in NodeJS 12 projects leads to cascading (and hard-to-decipher) build errors in downstream text editing apps like slap. Underlying reasons are build configuration issues, an incompatible upstream dependency and code style issues, and are all being adressed by the PR.

👟 Steps to Reproduce (S2R)

  1. Install node v12.9.0 and npm v6.11.2
  2. Try to install slap global binary:
    npm -g install 'https://github.com/cancerberoSgx/slap'

🤷🏽‍♂️ Main Issues

  • the referenced older version of atom/substring causes problems in a nodejs 12 environment
  • the required build tools are not correctly referenced
  • the package does not compile coffeescript automatically when installing from Git
  • the linting errors thwart successful build completion

🎯 Conducted Changes

  • scripts tooling Pprevious references to globally installed binaries were removed, as this mandated a trial-and-error approach when trying to install the required utilities. the scripts now simply uses npx to launch the enclosed node_modules binaries of coffeescript, rimraf or cpy
  • scripts phases Replaced the prepublish phase because it does not get called when installing modules from Git (and due to deprecation). The prepare phase now used initiates the compilation process during installations regardless of context.
  • package dependencies Replacement of the atom/superstring semver dependency reference with a direct Git-Commit-URL, which points to the minimum commit required for NodeJS 12 compatibility
  • package dependencies Upgrades of the other dependencies to current standards
  • Code Style Resolves code style issues as demanded by the linter phase.

🏡 Context

Version Tested
v13.17.0

Installation Approach
nvm, npm

Operating System
Apple macOS Mojave 10.14.6

…rom GitHub to incorporate the fixes required for Nodejs 12 compatibility. upgrades other dependencies to modern standards.
…endenciy modules (via npx) instead of referencing global namespace apps. also replaces the deprecated "prepublish" lifecycle phase with the newer "prepare" phase to ensure that the compilation is also done reliably when installing from Git oder locally (see github.com/npm/npm/issues/10074)
…nimum version of the substring package required to fully support Nodejs 12 environments
@rsese
Copy link

rsese commented Aug 27, 2019

Thanks @sidneys! And thanks for the detailed pull request description, it's always helpful - however, I'm not sure if there was an issue with GitHub displaying our templates when you created the PR but can you use our existing PR template?

Also, I see there are CI failures - do you think they're unrelated to your changes? If so, can you leave a comment explaining why the failures are unrelated as mentioned in our CONTRIBUTING guide?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants