update dependencies and adapt tests #21

hakandilek · 2018-05-30T15:02:49Z

trying to fix #19

rreusser · 2018-05-30T17:09:22Z

Looks reasonable. I'll see if I can set aside some time this afternoon and give it a test run since the tests don't always cover every possibility.

etpinard · 2018-05-30T17:17:41Z

test/fill.js

@@ -1,4 +1,4 @@
-var cwise = require("cwise")
+var cwise = require("..")


I'm interesting in knowing why this line needed to be patched.

Wouldn't this have been testing the npm-resolved copy rather than the local code? Seems a bit fishy.

Ohh, unless it's required to get static-module to correctly locate and replace require('cwise')…

Ohhh, I get it. It aliases node_modules/cwise to .., probably so that static module works correctly. That means npm run pretest is required before npm run test.

Relevant line here: https://github.com/scijs/cwise/blob/master/package.json#L23

Thanks for investigating @rreusser 🔬

It aliases node_modules/cwise to .., probably so that static module works correctly.

It would be nice to have someone confirming this.

That line with pretest is os dependent but is obsolete with '..'

@hakandilek I think you might be right.

Reasons I think it's an issue: this extra indirection is present, which suggests it may be needed. static-module is a dependency and treats require('..') differently from require('cwise'), even if they resolve to the same thing.

Reasons I question whether it's an issue: it doesn't immediately jump out to me that the tests actually hit static-module.

Seems alright to me to use require('..') if the tests pass and if linking this to and testing this with a real-world project succeeds.

hakandilek · 2018-05-30T18:29:25Z

Yeah I thought it's better to get it tested on the CI pipeline before it's distributed, and even merged to the master (as PR).

…

On Wed, 30 May 2018, 20:15 Ricky Reusser, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/fill.js <#21 (comment)>: > @@ -1,4 +1,4 @@ -var cwise = require("cwise") +var cwise = require("..") Wouldn't this have been testing the npm-distributed copy rather than the local code? Seems a bit fishy. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBdWajSbLMfLsgAx9K_QPau8mw1AYnvks5t3uHIgaJpZM4UTdmO> .

etpinard · 2018-05-30T20:13:36Z

I would be nice to test this branch on some "real world" apps. I guess I should try building plotly.js off this branch. Does anyone know a nice way to npm link third-party deduped modules?

hakandilek · 2018-05-30T20:23:48Z

@etpinard what about linking this repo under the sub-dependency path under node_modules dir of a real world plotly app?

etpinard · 2018-05-31T17:40:34Z

My first attempt didn't go well.

For some reason

$ (cwise) npm link
$ (plotly.js) npm link cwise
$ (plotly.js) budo lib/index.js --open

gives:

Much more granuarly using ndarray-fill

$ (cwise) npm link
$ (ndarray-fill) npm link cwise
$ (ndarray-fill) npm test

gives

hakandilek · 2018-06-14T07:33:38Z

@etpinard I think the problem is how browserify is integrated in the ndarray-fill. It fails to place the necessary require("cwise") block.

I've created a pull request integrating tape-run. It runs perfectly fine.

etpinard · 2018-06-14T13:52:04Z

I've created a pull request integrating tape-run. It runs perfectly fine.

Thanks for making that PR. That looks like a way more robust way to test cwise-transformed bundles 👏

But, this doesn't address this issue I noticed in plotly.js:

$ (cwise) npm link
$ (plotly.js) npm link cwise
$ (plotly.js) budo lib/index.js --open

still fails.

I understand you might not be interested in fixing this particular use case, so we certainly could merge this PR and release under a new major version signaling a possible breaking change.

hakandilek · 2018-06-15T11:54:53Z

@etpinard I think it's a problem with browserify.

I've tried it with webpack as described here, and it works.

Check out my plotly-webpack fork:

etpinard · 2018-06-15T13:08:19Z

Can you try a 3D graph? The scatter trace doesn't need to use cwise.

hakandilek · 2018-06-15T14:06:21Z

Yes you're right, it's failing. 😒

bung87 · 2018-10-19T08:59:27Z

I'd like to see this PR gets merged.so that 58 packages depends on this project wont receive vulnerabilities alert.

dy · 2018-12-18T19:49:05Z

@rreusser shall we proceed? This PR does not seem to introduce any breaking change.

rreusser · 2018-12-18T20:07:32Z

Hmm… @dy please do correct me if I'm mistaken, but I was under the impression that the current functioning of the cwise transform is incompatible with static-eval ^2, which is to say that cwise works fine with static-eval upgraded, but only if you're alright including esprima (~120kb, can't remember if that's minified or not) in production.

See: browserify/static-module#48 (comment)

Sceat · 2018-12-22T15:59:48Z

any eta ?

kibertoad · 2019-08-04T16:13:14Z

Can this be merged?

mhldtna · 2020-06-18T18:32:03Z

What is the status of this?

My application uses vue-plotly^1.1.0 which has an indirect dependency on cwise^1.0.10 through [email protected], [email protected] and [email protected], and [email protected]. A bunch of npm audit vulnerabilities are due to [email protected] use of static-module^1.0.0 which itself is dependent upon static-eval~0.2.0 which has moderate vulnerabilities according to npm audit.

The current version of static-eval is 2.1.0. Please fix this dependency.

kgryte · 2020-06-18T18:39:46Z

@mhldtna If you want to accelerate the process, feel free to investigate and return back with your findings. Contributions are welcome.

mhldtna · 2020-06-18T19:00:35Z

@kgryte - see browserify/static-module#48.

It's unrealistic for me to add any value to that discussion.

hakandilek added 2 commits May 30, 2018 16:58

update dependencies and adapt tests

0a1a46a

fix travis build & add all other nodejs versions

d3f955b

etpinard reviewed May 30, 2018

View reviewed changes

use static-module v 3.0.0

f0a1300

etpinard mentioned this pull request Jul 18, 2018

npm audit security report: several vulnerabilities found plotly/plotly.js#2822

Closed

WieserSoftware mentioned this pull request Nov 16, 2018

Moderate vulnerability in static-module v.1 used #24

Open

etpinard mentioned this pull request Nov 19, 2018

Upgrading dependencies to use static-eval 2 as well as some other newer modules plotly/plotly.js#3267

Closed

k-sai-kiranmayee mentioned this pull request Sep 25, 2019

potential security vulnerability via an outdated version of [email protected] > [email protected] #19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update dependencies and adapt tests #21

update dependencies and adapt tests #21

hakandilek commented May 30, 2018

rreusser commented May 30, 2018

etpinard May 30, 2018

rreusser May 30, 2018 •

edited

Loading

rreusser May 30, 2018

rreusser May 30, 2018

rreusser May 30, 2018

etpinard May 30, 2018

hakandilek May 30, 2018

rreusser May 30, 2018 •

edited

Loading

hakandilek commented May 30, 2018 via email

etpinard commented May 30, 2018 •

edited

Loading

hakandilek commented May 30, 2018

etpinard commented May 31, 2018

hakandilek commented Jun 14, 2018

etpinard commented Jun 14, 2018

hakandilek commented Jun 15, 2018

etpinard commented Jun 15, 2018

hakandilek commented Jun 15, 2018

bung87 commented Oct 19, 2018

dy commented Dec 18, 2018

rreusser commented Dec 18, 2018 •

edited

Loading

Sceat commented Dec 22, 2018

kibertoad commented Aug 4, 2019

mhldtna commented Jun 18, 2020

kgryte commented Jun 18, 2020

mhldtna commented Jun 18, 2020

		@@ -1,4 +1,4 @@
		var cwise = require("cwise")
		var cwise = require("..")

update dependencies and adapt tests #21

Are you sure you want to change the base?

update dependencies and adapt tests #21

Conversation

hakandilek commented May 30, 2018

rreusser commented May 30, 2018

etpinard May 30, 2018

Choose a reason for hiding this comment

rreusser May 30, 2018 • edited Loading

Choose a reason for hiding this comment

rreusser May 30, 2018

Choose a reason for hiding this comment

rreusser May 30, 2018

Choose a reason for hiding this comment

rreusser May 30, 2018

Choose a reason for hiding this comment

etpinard May 30, 2018

Choose a reason for hiding this comment

hakandilek May 30, 2018

Choose a reason for hiding this comment

rreusser May 30, 2018 • edited Loading

Choose a reason for hiding this comment

hakandilek commented May 30, 2018 via email

etpinard commented May 30, 2018 • edited Loading

hakandilek commented May 30, 2018

etpinard commented May 31, 2018

hakandilek commented Jun 14, 2018

etpinard commented Jun 14, 2018

hakandilek commented Jun 15, 2018

etpinard commented Jun 15, 2018

hakandilek commented Jun 15, 2018

bung87 commented Oct 19, 2018

dy commented Dec 18, 2018

rreusser commented Dec 18, 2018 • edited Loading

Sceat commented Dec 22, 2018

kibertoad commented Aug 4, 2019

mhldtna commented Jun 18, 2020

kgryte commented Jun 18, 2020

mhldtna commented Jun 18, 2020

rreusser May 30, 2018 •

edited

Loading

rreusser May 30, 2018 •

edited

Loading

etpinard commented May 30, 2018 •

edited

Loading

rreusser commented Dec 18, 2018 •

edited

Loading