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

Add d3-transition to the list of dependencies #160

Merged
merged 1 commit into from Jul 27, 2019

Conversation

mj3cheun
Copy link
Contributor

@mj3cheun mj3cheun commented Jul 23, 2019

When trying to build dash-bio with the newest version of ideogram (upgrading from 1.4.1), I get an error when changing layout. A screenshot of the error is linked below. I still don't fully understand the cause of the problem but I believe it has something to do with an update to the d3-selection library between versions 1.3.0 and all versions above. Basically below that version, everything works fine. Above, the d3-transition dependency needs to be specified in ideogram.

Screenshot from 2019-07-23 15-36-24

Note: this appears to only be a problem when using ideogram itself as a dependency.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.588% when pulling 6cfff0e on plotly:add-transition-dep into 18eb817 on eweitz:master.

@eweitz
Copy link
Owner

eweitz commented Jul 26, 2019

Thanks Manfred, great find.

I believe it has something to do with an update to the d3-selection library between versions 1.3.0 and all versions above. Basically below that version, everything works fine. Above, the d3-transition dependency needs to be specified in ideogram.

Odd! d3-brush requires d3-transition per Ideogram's package-lock.json, so Ideogram has historically pulled in this dependency implicitly.

Updating d3-brush seems like it would be the ideal fix for this bug.

Also, a test for this in web-test.js would help prevent regressions.

I aim to release a new version of Ideogram late next week. Those refinements would be nice, but if they're not feasible I'll merge this PR as it exists around then.

@mj3cheun
Copy link
Contributor Author

Interesting thing is that when ideogram is pulled as a dependency d3-brush is using the latest version, that is 1.0.6. I'm not sure if thats the issue.

Again I don't fully understand whats going on, but for some reason ideogram is not getting its own copy of d3-transition and neither is d3-brush. However, there is a d3-transition folder in dash-bio's node_modules folder from either ideogram or another library being used. Ideogram seems to require a copy of d3-transition in its own node_modules folder. Thus explicitly specifying d3-transition as a dependency and thus forcing ideogram to pull a copy of d3-transition into its own node_modules folder seems to solve the problem.

I have no idea how to write a test for this. For one thing, if you do a npm install in the ideogram folder, d3-transition gets pulled, solving the issue. Its only when ideogram is being pulled in as a dependency itself that this problem shows up. Therefore this problem will never show up in ideagram's tests.

@eweitz
Copy link
Owner

eweitz commented Jul 27, 2019

Again I don't fully understand whats going on, but for some reason ideogram is not getting its own copy of d3-transition and neither is d3-brush. However, there is a d3-transition folder in dash-bio's node_modules folder from either ideogram or another library being used. Ideogram seems to require a copy of d3-transition in its own node_modules folder. Thus explicitly specifying d3-transition as a dependency and thus forcing ideogram to pull a copy of d3-transition into its own node_modules folder seems to solve the problem.

Lacking peer dependency it's what's going on here, upon further investigation based on that helpful debugging information. Specifically, d3-selection should be a peer dependency of d3-transition. See d3/d3-transition#92 for details.

The update you've submitted does exactly that. Kudos on independently re-discovering that upstream bug and its recommended fix!

It's satisfying to have a root cause analysis.

I have no idea how to write a test for this. For one thing, if you do a npm install in the ideogram folder, d3-transition gets pulled, solving the issue. Its only when ideogram is being pulled in as a dependency itself that this problem shows up. Therefore this problem will never show up in ideagram's tests.

Excellent summary. I suppose bugs like this could be caught by higher-level tests that install ideogram as part of another package, and verifying behavior of D3 transitions there.

But I don't think such a complicated test is worthwhile.

Thanks for the fix, and for helping get to the bottom of this complex bug!

@eweitz eweitz merged commit a31d520 into eweitz:master Jul 27, 2019
@eweitz eweitz mentioned this pull request Sep 24, 2020
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.

None yet

3 participants