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

Distribution includes d3 functions, which get duplicated (& maybe version skew) when sites also include d3.js #203

Open
mhansen opened this issue Dec 18, 2021 · 2 comments
Assignees
Labels
bug review Tagged for review.

Comments

@mhansen
Copy link
Contributor

mhansen commented Dec 18, 2021

I think flamegraph/dist bundles a copy of many d3 functions, increasing page weight and risking version skew.

To Reproduce

  • Open https://cdn.jsdelivr.net/npm/d3-flame-graph@4.1.3/dist/d3-flamegraph.js
  • Search for node_modules/d3-
  • note 139 matches, including (a selection of files from top-level modules):
    • node_modules/d3-selection/src/selector.js
    • node_modules/d3-format/src/exponent.js
    • node_modules/d3-hierarchy/src/partition.js
    • node_modules/d3-array/src/ticks.js
    • node_modules/d3-color/src/define.js
    • node_modules/d3-interpolate/src/basis.js
    • node_modules/d3-scale/src/init.js
    • node_modules/d3-ease/src/cubic.js
    • node_modules/d3-dispatch/src/dispatch.js
    • node_modules/d3-timer/src/timer.js
    • node_modules/d3-transition/src/interrupt.js

I think this is true of the minified JS (but to a lesser extent because unused functions will be removed). I'm just using the unminified JS as it clearly demonstrates the files included.

For example, d3-flamegraph.js has this line from d3-transition/src/transition/schedule.js:

  if (schedule.state > CREATED) throw new Error("too late; already scheduled");

d3-flamegraph.min.js also seems to include this:

if(e.state>0)throw new Error("too late; already scheduled");

Expected behavior

  • No d3 functions included, because:
    • The expectation is on the user to include d3.js separately on the webpage, and we want just one version of each d3 function included (to reduce page weight).
    • When used in npm, importing d3-flame-graph imports the dist version, which might (not sure) prevent npm from deduplicating dependencies (say, if your project depends on d3-select and d3-flame-graph, you'll get two versions of d3-select in the output).

I wonder if this might lead to version skew sometimes if d3-flame-graph bundles one version of d3's functions and the site includes another? I suppose this is unavoidable in general; d3-flame-graph calls d3 functions, and expects some set of functions to be there.

I'm not sure the best way to solve this. Here's some brainstorming:

  • Maybe in npm, don't ship a dist version, but rather ship the raw source, depending on d3 libraries? This would prevent duplicating in npm builds. But if we're serving straight from npm to websites, this might break the websites.
  • Maybe we could keep the dist folder for websites, but for npm imports, don't use the dist version? Like in the top-level index.js, don't export all of dist/.
@mhansen
Copy link
Contributor Author

mhansen commented Dec 18, 2021

I should add, I don't know that this is an urgent issue or anything (nothing seems to be really 'breaking' yet!), just thought I'd file it to track in case this was unexpected.

@spiermar
Copy link
Owner

Thanks for opening the issue @mhansen! This is likely related to a bundling configuration we might have missed during the migration to Webpack, where we need to specify packages that are available already, so they don't get bundled and duplicated.
No reports of issues so far (and assuming the same major version is being used, should be fine), but we shouldn't be shipping duplicated code. I'll check the build configs to see if there's anything missing.

@spiermar spiermar added bug review Tagged for review. labels Dec 19, 2021
@spiermar spiermar self-assigned this Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug review Tagged for review.
Projects
None yet
Development

No branches or pull requests

2 participants