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

Update the minimum supported nodejs version to v10 and fix bugs #28

Merged
merged 5 commits into from
Jan 19, 2021
Merged

Update the minimum supported nodejs version to v10 and fix bugs #28

merged 5 commits into from
Jan 19, 2021

Conversation

rxliuli
Copy link
Contributor

@rxliuli rxliuli commented Jan 18, 2021

Note: May contain some bugs, mainly due to two reasons

  1. replacing replaceAll with replace
  2. replace ? for ||

There are also some non-dangerous replacements
For example, appending the fs-extra module to Promise fs-related operations


I created this PR to illustrate how easy it is to support nodejs10. I simply modified a few files. may cause some errors, but I have used this version to generate personal project documents, and it seems that there is no problem at the moment: https://util.liuli.moe/

ref: #20

Note: May contain some bugs, mainly due to two reasons
1. replacing replaceAll with replace
2. replace ? for ||

There are also some non-dangerous replacements
For example, appending the fs-extra module to Promise fs-related operations
@rxliuli
Copy link
Contributor Author

rxliuli commented Jan 18, 2021

May need a better way to replace replaceAll, as for ??, may need a utility function?

@pklaschka
Copy link
Member

First of all: Thank you very much for your contribution. I'll review this asap.

I believe that the replacement for replaceAll works alright since, by using a /g RegEx pattern (at least from the top of my head, I believe it to be that way) replaces all occurrences, anyway.

With ??, I'll have to review the occurrences and look into whether falsy values other than undefined or null are possible in the review process.

@pklaschka pklaschka added 🌷 enhancement New feature or request dependencies Pull requests that update a dependency file labels Jan 18, 2021
@pklaschka pklaschka modified the milestones: v0.4.3, v0.5.0 Jan 18, 2021
@pklaschka
Copy link
Member

Alright. Results from my initial review (I'll "GitHub-Review" when I've had the chance to test this more thoroughly, this is just the state of my review, as of now 🙂):

  • the ?. in the templates (variable.eta, line 14) also won't work in older versions
  • || instead of ?? in templates causes no problems, according to my initial tests. Falsy literal values, such as '' actually result in "''", making it no empty string and, thus, not falsy.
  • the package-lock.json is out of date. @rxliuli Please run npm install to bring it up to date with the changes in package.json

@codeclimate
Copy link

codeclimate bot commented Jan 19, 2021

Code Climate has analyzed commit e9dd5ba and detected 0 issues on this pull request.

View more on Code Climate.

@rxliuli
Copy link
Contributor Author

rxliuli commented Jan 19, 2021

Alright. Results from my initial review (I'll "GitHub-Review" when I've had the chance to test this more thoroughly, this is just the state of my review, as of now 🙂):

  • the ?. in the templates (variable.eta, line 14) also won't work in older versions
  • || instead of ?? in templates causes no problems, according to my initial tests. Falsy literal values, such as '' actually result in "''", making it no empty string and, thus, not falsy.
  • the package-lock.json is out of date. @rxliuli Please run npm install to bring it up to date with the changes in package.json

It has been modified. In addition, I merged the upstream modification (ie the latest commit of this repository) and resolved the conflict.

Copy link
Member

@fussel178 fussel178 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your contribution! 😄

Copy link
Member

@pklaschka pklaschka left a comment

Choose a reason for hiding this comment

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

LGTM, I'll go ahead and merge it 🙂

Thank you very much for your contribution.

@pklaschka pklaschka merged commit e586b86 into fliegwerk:main Jan 19, 2021
pklaschka added a commit that referenced this pull request Jan 19, 2021
…ersion to v10 and fix bugs (#28)"

This reverts commit e586b86.
pklaschka pushed a commit that referenced this pull request Jan 19, 2021
Co-authored-by: rxliuli <rxliuli@gmail.com>
pklaschka pushed a commit that referenced this pull request Jan 19, 2021
Co-authored-by: rxliuli <rxliuli@gmail.com>
Closes: #20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file 🌷 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It is recommended to support lower nodejs versions, for example, nodejs 10/12, nodejs 15 is really new
3 participants