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 dependencies and adapt tests #21

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

Conversation

hakandilek
Copy link

trying to fix #19

@rreusser
Copy link
Member

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member

@rreusser rreusser May 30, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@rreusser rreusser May 30, 2018

Choose a reason for hiding this comment

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

@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
Copy link
Author

hakandilek commented May 30, 2018 via email

@etpinard
Copy link
Contributor

etpinard commented May 30, 2018

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
Copy link
Author

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

@etpinard
Copy link
Contributor

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:

image

Much more granuarly using ndarray-fill

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

gives

image

@hakandilek
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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

image

@etpinard
Copy link
Contributor

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

@hakandilek
Copy link
Author

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

image

@bung87
Copy link

bung87 commented Oct 19, 2018

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

@dy
Copy link
Member

dy commented Dec 18, 2018

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

@rreusser
Copy link
Member

rreusser commented Dec 18, 2018

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
Copy link

Sceat commented Dec 22, 2018

any eta ?

@kibertoad
Copy link

Can this be merged?

@mhldtna
Copy link

mhldtna commented Jun 18, 2020

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
Copy link

kgryte commented Jun 18, 2020

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

@mhldtna
Copy link

mhldtna commented Jun 18, 2020

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

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

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.

potential security vulnerability via an outdated version of [email protected] > [email protected]
9 participants