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

Don't export all of d3 #4379

Open
bhousel opened this issue Sep 27, 2017 · 5 comments
Open

Don't export all of d3 #4379

bhousel opened this issue Sep 27, 2017 · 5 comments
Labels
chore-build Improvements to the iD build scripts / CI environment waitfor-upstream Waiting for something in an upstream project

Comments

@bhousel
Copy link
Member

bhousel commented Sep 27, 2017

followup from #4372 (comment)

We're currently doing this in our iD index.js:

import * as d3 from 'd3';
export { d3 };

So that later we can use it from our spec_helper.js so that tests can use it.

window.d3 = iD.d3;   // TODO: remove

Instead our test harness should be able to require d3 separately in index.html:

<script src='../node_modules/d3/build/d3.js'></script> 

... and that mostly works, except some of the tests that use events don't like it at all. d3.event is special because it's like a global event reference. The d3 docs have this to say:

If you use Babel, Webpack, or another ES6-to-ES5 bundler, be aware that the value of d3.event changes during an event! An import of d3.event must be a live binding, so you may need to configure the bundler to import from D3’s ES6 modules rather than from the generated UMD bundle; not all bundlers observe jsnext:main. Also beware of conflicts with the window.event global.

☝️ So I think this is an actual problem for how the tests are written, and we should figure out what this means and how to do the thing. I did a bit of debugging, but it involves digging into what the bundled d3 code is doing, because the sourcemaps lie.

@bhousel bhousel added chore-dependency Improvements to one of iD's dependencies chore-build Improvements to the iD build scripts / CI environment new contributor opportunity Best for first-time contributors without prior experience. You won’t be assigned; PRs welcome! Hacktoberfest and removed chore-dependency Improvements to one of iD's dependencies labels Sep 27, 2017
@bhousel bhousel added help wanted For intermediate contributors, requires investigation or knowledge of iD code and removed new contributor opportunity Best for first-time contributors without prior experience. You won’t be assigned; PRs welcome! labels Oct 12, 2017
@darthgera123
Copy link

ill work on this issue

@darthgera123
Copy link

when i make the following changes obviously by removing the import command, the entire site goes blank. So what exactly do i need to do, could you please guide me

@bhousel
Copy link
Member Author

bhousel commented Nov 2, 2017

when i make the following changes obviously by removing the import command, the entire site goes blank. So what exactly do i need to do, could you please guide me

I would expect that removing the

import * as d3 from 'd3';
export { d3 };

...should not make the whole site go blank - but it should make several of the tests fail.

What kinds of errors do you see in the JavaScript console?

@darthgera123
Copy link

darthgera123 commented Nov 2, 2017

i apologize, didnt remove export {d3} earlier. Its more or less working fine.

@bhousel bhousel removed the help wanted For intermediate contributors, requires investigation or knowledge of iD code label Jan 23, 2020
@bhousel
Copy link
Member Author

bhousel commented Jan 23, 2020

I spent some time on this today and yesterday.
What's happening when we try to include d3 in a <script> tag is that the tests end up with a separate d3 from the iD internally bundled one.

This mostly works except for d3.event, which they don't share.

So for example, we have a test for the hover behavior. It simulates a mouse event.

That mouse event ends up handled by window.d3:
Screenshot 2020-01-23 11 52 52

instead of iD's bundled d3:
Screenshot 2020-01-23 11 53 08

So the copy of d3.event that iD is looking at doesn't get updated.

I can't think of a way to get these things working together. Maybe everywhere we import a d3_event in the iD code and try to use it, we'd add a fallback to window.d3.event. Or - not bundle d3 at all and always treat it external (loaded in a script tag). Neither of these are great.

It looks like d3.event is going away in the D3 v6 coming soonish, so it's not worth spending more time on this now since we'll have to change the code anyway.

@bhousel bhousel added the waitfor-upstream Waiting for something in an upstream project label Jan 24, 2020
bhousel added a commit that referenced this issue Feb 22, 2020
(re: #4379)

This trims a bit more off the iD bundle size
zlavergne pushed a commit to KaartGroup/iD that referenced this issue Mar 10, 2020
(re: openstreetmap#4379)

This trims a bit more off the iD bundle size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore-build Improvements to the iD build scripts / CI environment waitfor-upstream Waiting for something in an upstream project
Projects
None yet
Development

No branches or pull requests

2 participants