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

929 shift to npm approach map styles #32

Draft
wants to merge 11 commits into
base: staging
Choose a base branch
from

Conversation

erictheise
Copy link
Member

OpenHistoricalMap/issues#929 was opened in response to the different approaches that have been taken to integrate map-styles into ohm-website, other OpenHistoricalMap projects, and non-OHM projects. This draft pull request does a few things:

  • runs an npm init, thus adding a package.json file
  • adds a prepare script that can be run manually during development and that runs automatically as part of npm publish
  • renames styles for internal consistency within map-styles and in ohm-website (e.g. entities known variously as main or original are now called historical to match the layer picker on the website)

The prepare script functions similarly to the update_map_styles.py script in ohm-deploy but is used earlier in the workflow allowing style versioning in ohm-website and openhistoricalmap-embed to be better understood.

A companion branch and PR will be opened in ohm-website momentarily.

@erictheise
Copy link
Member Author

Version 0.9.2 published at https://www.npmjs.com/package/@openhistoricalmap/map-styles. The styles are mature but the module isn't so I'd only bump to 1.0.0 once we're convinced the approach is sound and the integration with ohm-website works.

LICENSE Outdated Show resolved Hide resolved
Copy link
Member

@1ec5 1ec5 Feb 6, 2025

Choose a reason for hiding this comment

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

renames styles for internal consistency within map-styles and in ohm-website (e.g. entities known variously as main or original are now called historical to match the layer picker on the website)

Will each of the stylesheets’ public URLs change with respect to what we currently document? The last time we changed the URLs to these styles, it created quite a ripple effect: OpenHistoricalMap/issues#792 (comment). historical.json is a more descriptive name, and the new names of the other files are cleaner, but if these changes have any impact beyond this repository, we should save them for a different task separate from migrating to NPM.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll argue that migrating to NPM (and a halving of the cartography team) gives us a unique opportunity to do a bit of housecleaning as regards filenames. git got confused between the root level main directory and the branch of the same name, searching codebases for railway rather than rail narrows results considerably, and the one file that began with ohm suggested that it was unique in some way but I couldn't detect any reason why that would be so. The forEach loop in prepare expects the filenames to adhere to the naming in the styles array.

Copy link
Member

@1ec5 1ec5 Feb 6, 2025

Choose a reason for hiding this comment

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

I agree that these changes make sense. However, if the URL https://www.openhistoricalmap.org/map-styles/main/main.json breaks, then it’s a backwards-incompatible change and we need to plan accordingly. If you’re up for tackling this increased scope, then let’s track the various steps explicitly in a ticket. Since the last renaming, multiple third parties have adopted these styles, and there may be others we don’t know about.

It might be worth investigating whether we can put in symlinks or HTTP redirects to keep the old paths or URLs working for the time being as a deprecation strategy.

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

Successfully merging this pull request may close these issues.

2 participants