update dependencies and adapt tests

[hakandilek]

trying to fix #19

[rreusser]

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]

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

[rreusser]

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

[rreusser]

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

[rreusser]

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.

[rreusser]

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

[etpinard]

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.

[hakandilek]

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

[rreusser]

@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]

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]

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]

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

[etpinard]

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]

@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]

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]

@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]

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

[hakandilek]

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

[bung87]

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

[dy]

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

[rreusser]

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]

any eta ?

[kibertoad]

Can this be merged?

[mhldtna]

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 plotly.js@1.52.1, gl-plot2d@1.4.4 and glplot3d@2.4.5, and gl-select-static@2.0.6. A bunch of npm audit vulnerabilities are due to cwise@1.0.10 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]

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

[mhldtna]

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

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