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

Fetch frequently updated resources instead of bundling them #4994

Closed
bhousel opened this issue Apr 17, 2018 · 17 comments
Closed

Fetch frequently updated resources instead of bundling them #4994

bhousel opened this issue Apr 17, 2018 · 17 comments
Assignees
Labels
Milestone

Comments

@bhousel
Copy link
Member

@bhousel bhousel commented Apr 17, 2018

iD bundles a few dependencies from other projects:

That means that sometimes these other projects will receive updates, but users of iD will not get to see the update until the next time I release a new version of iD.

It would be great if we can figure out a way to have the released iD on openstreetmap.org pull in more recent data from these other projects.

  • I could do this today for osm-community-index. This is because that project has releases that follow semantic versioning and are published to npm. We could always fetch the "latest" patch version from the unpkg CDN and know that it shouldn't break anything. We could also fallback to loading a local version if for some reason the CDN doesn't respond.

  • I could probably do this for name-suggestion-index, but it's a little more complicated because the data in that index needs some processing to turn the results into presets. So it would be, fetch the name suggestion file and then patch in the suggestion presets in the first few seconds while the user is using iD. It would be weird but might work.

  • I can't do this for editor-layer-index unless we do osmlab/editor-layer-index#214

  • I can't really update the translations, because these resources are linked to the current code in master branch. However it's worth mentioning that if we do find a way to pull in a more recent community index, it would be useful to pull in the translations that go with it. It's complicated.

See also #4019 #3591

Update: Also this blog about fetching a bunch of small files is the way to go in modern HTTP/2 capable browsers.

@bhousel bhousel added the bluesky label Apr 17, 2018
bhousel referenced this issue May 11, 2018
`npm install` may refuse to work if it finds the
`editor-layer-index/.git` folder
@bhousel bhousel self-assigned this Jan 28, 2020
@bhousel bhousel added enhancement wip and removed bluesky labels Jan 28, 2020
@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Jan 28, 2020

I'm going to do this now that the iD bundle has gotten big and builds are slow.
see #3403 (comment)

before

Screenshot 2020-01-27 17 23 35

I've added a coreData module for managing all the data that iD needs to work (the pink box). I'll move most of that stuff into the dist folder and fetch it at runtime instead of bundling it into iD.

@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Jan 30, 2020

I should probably be tagging this issue in the commits I'm doing, but:

  • wmf-sitematrix fetches from CDN as of 33a2daf
  • osm-community-index fetches from CDN as of fb4d658
bhousel added a commit that referenced this issue Jan 30, 2020
@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Jan 30, 2020

I did imagery today and that moved the needle a bunch:
Screenshot 2020-01-30 17 12 34

iD.js is now 8.6mb, of which 5.14mb is bundled data

bhousel added a commit that referenced this issue Feb 1, 2020
@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Feb 1, 2020

I removed the name-suggestion-index brand data today partially - we can fetch these from the CDN as well.

When I say partially - we were bundling NSI brand data just for the outdated_tags validation. Now that it's not bundled, we can run the tests with very minimal data - our tests run significantly faster now!

We are still using the NSI brands to generate presets, and these end up in presets.json, which is still bundled, so there is still a bunch of data to trim. 🔪

Screenshot 2020-01-31 22 05 36

iD.js is now 7.69mb, of which 4.15mb is bundled data.

@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Feb 1, 2020

Not bundled data, but today I also worked on getting rid that big turf-jsts box.
ideditor/location-conflation#1

@mmd-osm

This comment has been minimized.

Copy link
Contributor

@mmd-osm mmd-osm commented Feb 1, 2020

We could also fallback to loading a local version if for some reason the CDN doesn't respond.

This comment reminded me of some projects, such as POSM, that use iD in a more or less offline scenario (wrt fetching presets from some CDN). I'm wondering if there will be some easy way for those projects to override modules/core/data.js URLs and use their own (local) copy.

@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Feb 1, 2020

I'm wondering if there will be some easy way for those projects to override modules/core/data.js URLs and use their own (local) copy.

Yes, I wrote it so people can change the file map if they want to.. Thinking more about this, it might also be useful to record the version number and a link, so we can have some place in the app where people can see what versions of stuff that iD is using.

bhousel added a commit that referenced this issue Feb 1, 2020
@andrewharvey

This comment has been minimized.

Copy link
Contributor

@andrewharvey andrewharvey commented Feb 5, 2020

@bhousel what are the blockers for having editor-layer-index update more frequently? Can it not just pull directly from https://osmlab.github.io/editor-layer-index/imagery.json ?

@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Feb 5, 2020

Can it not just pull directly from https://osmlab.github.io/editor-layer-index/imagery.json ?

If we did that, any broken build would break imagery for everyone.
My plan is to just make our own version of the imagery index once things settle down.

@andrewharvey

This comment has been minimized.

Copy link
Contributor

@andrewharvey andrewharvey commented Feb 5, 2020

My plan is to just make our own version of the imagery index once things settle down.

I don't mean to take this off topic, if there is another issue discussing this, happy to move it there.

It would be nice to see some more details about what's planned by iD. I volunteer a lot of time and effort to help curate the editor-layer-index, the community is already fragmented with JOSM maintaining their own imagery index separate to editor-layer-index, so naturally if there's issues with editor-layer-index preventing it being ideal for iD please open a ticket on editor-layer-index I'd be more than happy try and help out.

bhousel added a commit that referenced this issue Feb 6, 2020
(re: #4994)
@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Feb 6, 2020

Today I finished moving the presets data external. Wow! The iD.js bundle picture has really changed:

Screenshot 2020-02-05 12 26 36

iD.js is now 5.35mb, of which 1.8mb is bundled data. We are finally mostly bundling actual source code. I will try to trim down the dependencies and whatever bundled data remains. The rest is mostly the English language strings.

The changes to the preset system have probably introduced some bugs, so I'll be testing a lot the next few days. #6840 and #6664 are related and I'll close them somehow.

At some point it would be nice to move the presets external into their own repo like the other data that iD depends on. This is #7130 and #2656. We now have name-suggestion-index and osm-community-index running smoothly so I'd probably do it similar.

@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Feb 6, 2020

if there's issues with editor-layer-index preventing it being ideal for iD please open a ticket on editor-layer-index I'd be more than happy try and help out.

We already did have an ELI issue for this (osmlab/editor-layer-index#214, which I mention above as a blocker), and I submitted a PR to fix that and all the other issues with ELI that we have (osmlab/editor-layer-index#490) and nobody wanted it. This was like 2 years ago.

If we do end up making a replacement imagery index, I'd definitely welcome your help on it.

@grischard

This comment has been minimized.

Copy link
Contributor

@grischard grischard commented Feb 6, 2020

It's funny how we all want to kill ELI in the end :).

I'd like to do two things this year:

  • Sanity check(s) before pushing a build: I was thinking pick a source at random, and make sure it's in the imagery.geojson we've produced. Open to any ideas that would let us be more confident that an imagery.geojson will never be broken.
  • Consider turning the imagery.geojson into some kind of metadata vector tiles, so that iD can fetch imagery only for the area it's interested in.

I don't want to hijack this ticket, so further discussion can happen on the ELI tracker if it isn't wanted here.

@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Feb 6, 2020

It's funny how we all want to kill ELI in the end :).

It's not that I want to kill it, but it's more that I don't think we're very well aligned on what we need it to do. This is Open Source, and I've tried fixing the problems with a PR, but at this point there is no use complaining about it when I could just make a thing that works better for us.

I'd like to do two things this year:

  • Sanity check(s) before pushing a build: I was thinking pick a source at random, and make sure it's in the imagery.geojson we've produced. Open to any ideas that would let us be more confident that an imagery.geojson will never be broken.

I think people should work under the assumption that master branch will get broken sometimes. I think doing actual releases with semantic version numbers is a good thing for anyone who sits downstream from your project.

  • Consider turning the imagery.geojson into some kind of metadata vector tiles, so that iD can fetch imagery only for the area it's interested in.

Fetching all the imagery isn't really a problem that we have. The complete imagery.min.json is down to 1.7mb and I can get it down much smaller by reusing geometries, simplifying, and leveraging country-coder and location-conflation.

I don't want to hijack this ticket, so further discussion can happen on the ELI tracker if it isn't wanted here.

Yeah, I feel like we've been through this already. Let's keep this ticket just about trimming the data from the iD bundle.

@bhousel bhousel added this to the Next Release milestone Feb 6, 2020
bhousel added a commit that referenced this issue Feb 7, 2020
coreData now owns all the data, and data/index.js should eventually go away
(re: #4994)
bhousel added a commit that referenced this issue Feb 21, 2020
bhousel added a commit that referenced this issue Feb 21, 2020
@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Feb 21, 2020

Today I finally removed the last data bundled from iD - the locale data and English strings. context.init() can initiate loading these things first before it does anything, then we can defer rendering in ui() until these data fetching Promises have settled.

The bundle picture for iD has shrunk considerably! 🎉

Screenshot 2020-02-21 12 27 26

  • iD.js is down to 4.5mb (1.93mb is our code, 1.7mb is dependencies - and there is some potential to trim there in d3 and jsts). Bundled data down to 886k (I'm assuming this is getting pulled in from a dependency like country-coder's borders.json file.. This is very ok! Not worth trimming further)
  • Build and test times are now down to around 15sec. Requiring rollup to process all that data was incredibly wasteful.
  • We have a lot more flexibility about how iD fetches data from elsewhere. We're already loading the name-suggestion-index and osm-community-index from CDN! So these things can now be released independently of iD. Soonish, I'm going to look into whether we can do this imagery and translations too.

I'm going to close this for now - happy with how it turned out 💪

@bhousel bhousel closed this Feb 21, 2020
bhousel added a commit that referenced this issue Feb 21, 2020
(re: #4994)
@quincylvania

This comment has been minimized.

Copy link
Collaborator

@quincylvania quincylvania commented Feb 21, 2020

@bhousel Awesome! This will make iD so much nimbler for everyone—developers through faster build times and swappable data, mappers through smaller download sizes and more-frequent data updates. Yet another great step toward modular iD 🤙

@bhousel

This comment has been minimized.

Copy link
Member Author

@bhousel bhousel commented Mar 11, 2020

Replacing editor-layer-index being tracked on #7425 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.