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 node@14, and pull in #209, #245, and #255 #323

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

Conversation

randytarampi
Copy link

@randytarampi randytarampi commented Apr 29, 2020

This builds on the work in #317 and https://github.com/mixer/lwip/releases/tag/v1.0.3, and is released under https://www.npmjs.com/package/@randy.tarampi/lwip.

This should also close #209, close #245, and close #255.

There's a whole host of changes here – notable ones called out in the CHANGELOG – some of which are breaking for the current state of the module, but who's using node<8 these days anyways?

@coveralls
Copy link

coveralls commented Apr 29, 2020

Coverage Status

Coverage increased (+1.1%) to 99.429% when pulling b1f9f87 on randytarampi:master into a559d24 on EyalAr:master.

@codecov-io
Copy link

codecov-io commented May 7, 2020

Codecov Report

Merging #323 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #323   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files           5        5           
  Lines         525      525           
=======================================
  Hits          522      522           
  Misses          3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 888bccb...888bccb. Read the comment docs.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@a559d24). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #323   +/-   ##
=========================================
  Coverage          ?   99.42%           
=========================================
  Files             ?        5           
  Lines             ?      525           
  Branches          ?        0           
=========================================
  Hits              ?      522           
  Misses            ?        3           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a559d24...8a18e40. Read the comment docs.

snyk-bot and others added 10 commits October 10, 2020 21:30
…db64933b15258

[Snyk] Security upgrade mocha from 8.1.3 to 8.2.0
…bbfe97a91c18f6476

[Snyk] Upgrade mocha from 8.1.3 to 8.2.0
…f8b4b952611c9bc15

[Snyk] Upgrade nan from 2.14.1 to 2.14.2
…ce9aeab98ba41c982

[Snyk] Upgrade semantic-release from 17.1.2 to 17.2.1
…dc50611715386e458

[Snyk] Upgrade eslint from 7.10.0 to 7.11.0
@codecov-io
Copy link

codecov-io commented Oct 18, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@a559d24). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #323   +/-   ##
=========================================
  Coverage          ?   99.42%           
=========================================
  Files             ?        5           
  Lines             ?      525           
  Branches          ?        0           
=========================================
  Hits              ?      522           
  Misses            ?        3           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a559d24...cea7d33. Read the comment docs.

randytarampi and others added 16 commits October 18, 2020 23:18
…7e6537c5feb599dce

[Snyk] Upgrade snyk from 1.405.1 to 1.406.0
I mean, the cloud service is scanning the GitHub repos anyways, so there's no need to run this locally right?

Or am I getting this switch the other way around...? 🤔
…7cb3241a73dcc5ee7

[Snyk] Upgrade snyk from 1.406.0 to 1.414.1
[skip ci]

## [3.0.12](v3.0.11...v3.0.12) (2020-10-18)

### Bug Fixes

* package.json & package-lock.json to reduce vulnerabilities ([bd42ca9](bd42ca9))
* upgrade eslint from 7.10.0 to 7.11.0 ([8f72cff](8f72cff))
* upgrade mocha from 8.1.3 to 8.2.0 ([9fbeaf2](9fbeaf2))
* upgrade nan from 2.14.1 to 2.14.2 ([9b8adc2](9b8adc2))
* upgrade semantic-release from 17.1.2 to 17.2.1 ([f812b98](f812b98))
* upgrade snyk from 1.405.1 to 1.406.0 ([74324ce](74324ce))
* upgrade snyk from 1.406.0 to 1.414.1 ([8b54df3](8b54df3))
…cd1b1e19103f0d93b

[Snyk] Upgrade snyk from 1.414.1 to 1.415.0
[skip ci]

## [3.0.13](v3.0.12...v3.0.13) (2020-10-20)

### Bug Fixes

* upgrade snyk from 1.414.1 to 1.415.0 ([296608b](296608b))
…bd4912b093b60fce9

[Snyk] Upgrade snyk from 1.415.0 to 1.416.0
[skip ci]

## [3.0.14](v3.0.13...v3.0.14) (2020-10-21)

### Bug Fixes

* upgrade snyk from 1.415.0 to 1.416.0 ([c48a917](c48a917))
randytarampi and others added 28 commits May 28, 2021 12:22
…65c2867ee95ebb0e80

[Snyk] Upgrade eslint from 7.26.0 to 7.27.0
…83d23fafae6433ae9c

[Snyk] Upgrade eslint-plugin-import from 2.23.2 to 2.23.3
…e1de0000cb2ef61bab

[Snyk] Upgrade snyk from 1.604.0 to 1.605.0
…5c5c01cfa8df10d919

[Snyk] Upgrade snyk from 1.605.0 to 1.616.0
…17678d760afe59661f

[Snyk] Upgrade eslint-plugin-import from 2.23.3 to 2.23.4
…26271034e8daee7103

[Snyk] Upgrade snyk from 1.616.0 to 1.618.0
…c546840c3ed8255ad2

[Snyk] Upgrade snyk from 1.618.0 to 1.620.0
…b767a7b51de72c4c4f

[Snyk] Upgrade snyk from 1.620.0 to 1.621.0
…53efe9025e96918238

[Snyk] Upgrade eslint from 7.27.0 to 7.28.0
…6ba008ad1dc2e4bfe1

[Snyk] Upgrade snyk from 1.621.0 to 1.622.0
…12af6f0ae679403b96

[Snyk] Upgrade snyk from 1.622.0 to 1.623.0
…87fe6369494e4276f1

[Snyk] Upgrade snyk from 1.623.0 to 1.624.0
…7ce6893ade7da5d87e

[Snyk] Upgrade coveralls from 3.1.0 to 3.1.1
…e4365ff4f553f7

[Snyk] Security upgrade snyk from 1.624.0 to 1.667.0
…dca5acb4bc8885

[Snyk] Security upgrade snyk from 1.667.0 to 1.685.0
@@ -126,6 +128,6 @@ NAN_METHOD(encodeToGifBuffer);
void pngWriteCB(png_structp png_ptr, png_bytep data, png_size_t length);
int gifWriteCB(GifFileType * gif, const GifByteType * chunk, int len);
void remapTransparentPixels(unsigned char * target, const unsigned char * map, size_t width, size_t height, int transColor, int threshold);
void initAll(Handle<Object> exports);
void initAll(v8::Local<v8::Object> exports);
Copy link

@caitp caitp Mar 29, 2022

Choose a reason for hiding this comment

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

Not an owner for this project or anything, but I'd personally appreciate it if we removed the using namespace <v8, node, etc>; statements above and make API namespaces explicit everywhere in the module, as is done here.

👍

Handle had been deprecated for years, so replacing it with Local just makes sense

Copy link

@caitp caitp left a comment

Choose a reason for hiding this comment

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

v8 API changes and library updates and abi compat abstractions look good to me.

Ultimately I'm more worried about the upstream port being usable again, so I'm not too concerned with these extra changes and would take it either way --- but IME, it's generally better for a project to add changes in decisive, purpose-driven or goal-oriented chunks, as it helps with things like codereview, finding the source of regressions or bugs, etc.

It isn't obvious from the github diff what is changing with the changed images. Are these binary changes required for tests to pass? It would be worth documenting this or else removing them from the diff


function Batch(image) {
const path = require('path'),
Copy link

Choose a reason for hiding this comment

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

I would try to avoid substantial refactoring like this in a change titled "support node@14", personally

@@ -1,70 +1,69 @@
var join = require('path').join,
assert = require('assert'),
const assert = require('assert'),
Copy link

Choose a reason for hiding this comment

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

many small refactorings in tests like this (var->lexical declarations, function expressions -> arrow functions, etc) can also be a challenge to review and might be better left to a followup

@@ -0,0 +1,35 @@
# EditorConfig is awesome: http://EditorConfig.org
Copy link

Choose a reason for hiding this comment

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

love editorconfig, not sure it fits with an ABI compat patch, personally. I'd take this in a separate PR

@@ -0,0 +1,6 @@
node_modules
Copy link

Choose a reason for hiding this comment

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

ditto

@@ -0,0 +1,33 @@
extends:
Copy link

Choose a reason for hiding this comment

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

ditto

@@ -1,6 +1,55 @@
build
Copy link

Choose a reason for hiding this comment

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

I'd avoid reordering this to stuff to keep the diff/blame easier to follow

@@ -0,0 +1,39 @@
{
Copy link

Choose a reason for hiding this comment

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

love code coverage, do we need to add it in this PR? I'd probably add this in a separate PR before this tbh

@@ -0,0 +1,3 @@
verbose = 5
Copy link

Choose a reason for hiding this comment

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

ditto

# Light-weight image processor for NodeJS

[![Join the chat at https://gitter.im/EyalAr/lwip](https://badges.gitter.im/Join%20Chat.svg)](https://gitter.im/EyalAr/lwip?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge&utm_content=badge)
[![npm versions](https://img.shields.io/npm/v/@randy.tarampi/lwip.svg?style=flat-square)](https://www.npmjs.org/package/@randy.tarampi/lwip)
Copy link

Choose a reason for hiding this comment

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

probably this stuff should point to the upstream version (speaking of, probably don't include downstream fork package renaming in the pull request for the upstream version). If it were up to me, I'd add these changes in separate PRs introducing these extras

[![Install @randy.tarampi/lwip](https://nodeico.herokuapp.com/@randy.tarampi/lwip.svg)](https://www.npmjs.com/package/@randy.tarampi/lwip)


This is a branch based off of [@kant2002/lwip](https://www.npmjs.com/package/@kant2002/lwip) and [@mcph/lwip](https://www.npmjs.com/package/@mcph/lwip), customized for my own use.
Copy link

Choose a reason for hiding this comment

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

It's worth crediting the original authors, but being an upstream branch, maybe "customized for my own use" stands out as strange

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

Successfully merging this pull request may close these issues.

7 participants