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.

Sign up for GitHub

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

Introduce the hash URL param presetUrlOverwrite #9475

Closed
wants to merge 2 commits into from
Closed

Introduce the hash URL param presetUrlOverwrite #9475

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d64839
Refactor url templates
tordans Jan 20, 2023
6f92a94
API: Introduce `presetUrlOverwrite` to enable testing id-tagging-sche…
tordans Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions config/id.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// cdns for external data packages
const presetsCdnUrl = 'https://cdn.jsdelivr.net/npm/@openstreetmap/id-tagging-schema@{presets_version}/';
const ociCdnUrl = 'https://cdn.jsdelivr.net/npm/osm-community-index@{version}/';
const wmfSitematrixCdnUrl = 'https://cdn.jsdelivr.net/npm/wmf-sitematrix@{version}/';
const nsiCdnUrl = 'https://cdn.jsdelivr.net/npm/name-suggestion-index@{version}/';
const presetsCdnUrlTemplate = 'https://cdn.jsdelivr.net/npm/@openstreetmap/id-tagging-schema@{presets_version}/';
const ociCdnUrlTemplate = 'https://cdn.jsdelivr.net/npm/osm-community-index@{version}/';
const wmfSitematrixCdnUrlTemplate = 'https://cdn.jsdelivr.net/npm/wmf-sitematrix@{version}/';
const nsiCdnUrlTemplate = 'https://cdn.jsdelivr.net/npm/name-suggestion-index@{version}/';

// api urls and settings
const osmApiConnections = [
Expand All @@ -20,10 +20,10 @@ const taginfoApiUrl = 'https://taginfo.openstreetmap.org/api/4/';
const nominatimApiUrl = 'https://nominatim.openstreetmap.org/';

export {
presetsCdnUrl,
ociCdnUrl,
wmfSitematrixCdnUrl,
nsiCdnUrl,
presetsCdnUrlTemplate,
ociCdnUrlTemplate,
wmfSitematrixCdnUrlTemplate,
nsiCdnUrlTemplate,
osmApiConnections,
taginfoApiUrl,
nominatimApiUrl,
Expand Down
20 changes: 12 additions & 8 deletions modules/core/file_fetcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import parseVersion from 'vparse';
import { presetsCdnUrl, ociCdnUrl, wmfSitematrixCdnUrl } from '../../config/id.js';
import { presetsCdnUrlTemplate, ociCdnUrlTemplate, wmfSitematrixCdnUrlTemplate } from '../../config/id.js';

import packageJSON from '../../package.json';

Expand All @@ -11,10 +11,15 @@ export { _mainFileFetcher as fileFetcher };
// coreFileFetcher asynchronously fetches data from JSON files
//
export function coreFileFetcher() {
const presetsVersion = packageJSON.devDependencies['@openstreetmap/id-tagging-schema'];
const presetsCdnUrl = presetsCdnUrlTemplate.replace('{presets_version}', presetsVersion);

const ociVersion = packageJSON.dependencies['osm-community-index'] || packageJSON.devDependencies['osm-community-index'];
const v = parseVersion(ociVersion);
const ociVersionMinor = `${v.major}.${v.minor}`;
const presetsVersion = packageJSON.devDependencies['@openstreetmap/id-tagging-schema'];
const ociCdnUrl = ociCdnUrlTemplate.replace('{version}', ociVersionMinor);

const wmfSitematrixCdnUrl = wmfSitematrixCdnUrlTemplate.replace('{version}', '0.1');

let _this = {};
let _inflight = {};
Expand All @@ -29,24 +34,23 @@ export function coreFileFetcher() {
'qa_data': 'data/qa_data.min.json',
'shortcuts': 'data/shortcuts.min.json',
'territory_languages': 'data/territory_languages.min.json',
'oci_defaults': ociCdnUrl.replace('{version}', ociVersionMinor) + 'dist/defaults.min.json',
'oci_features': ociCdnUrl.replace('{version}', ociVersionMinor) + 'dist/featureCollection.min.json',
'oci_resources': ociCdnUrl.replace('{version}', ociVersionMinor) + 'dist/resources.min.json',
'presets_package': presetsCdnUrl.replace('{presets_version}', presetsVersion) + 'package.json',
'oci_defaults': ociCdnUrl + 'dist/defaults.min.json',
'oci_features': ociCdnUrl + 'dist/featureCollection.min.json',
'oci_resources': ociCdnUrl + 'dist/resources.min.json',
'presets_package': presetsCdnUrl + 'package.json',
'deprecated': presetsCdnUrl + 'dist/deprecated.min.json',
'discarded': presetsCdnUrl + 'dist/discarded.min.json',
'preset_categories': presetsCdnUrl + 'dist/preset_categories.min.json',
'preset_defaults': presetsCdnUrl + 'dist/preset_defaults.min.json',
'preset_fields': presetsCdnUrl + 'dist/fields.min.json',
'preset_presets': presetsCdnUrl + 'dist/presets.min.json',
Comment on lines 48 to 53
Copy link
Member

@tyrasd tyrasd Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These files (except presets_package) should use the presetsCdnUrlTemplate instead of presetsCdnUrlTemplate. See my comment above.

Copy link
Member

Choose a reason for hiding this comment

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

For example:

Suggested change
'deprecated': presetsCdnUrl + 'dist/deprecated.min.json',
'discarded': presetsCdnUrl + 'dist/discarded.min.json',
'preset_categories': presetsCdnUrl + 'dist/preset_categories.min.json',
'preset_defaults': presetsCdnUrl + 'dist/preset_defaults.min.json',
'preset_fields': presetsCdnUrl + 'dist/fields.min.json',
'preset_presets': presetsCdnUrl + 'dist/presets.min.json',
'deprecated': (presetsUrlOverwrite || presetsCdnUrlTemplate) + 'dist/deprecated.min.json',
'discarded': (presetsUrlOverwrite || presetsCdnUrlTemplate) + 'dist/discarded.min.json',
'preset_categories': (presetsUrlOverwrite || presetsCdnUrlTemplate) + 'dist/preset_categories.min.json',
'preset_defaults': (presetsUrlOverwrite || presetsCdnUrlTemplate) + 'dist/preset_defaults.min.json',
'preset_fields': (presetsUrlOverwrite || presetsCdnUrlTemplate) + 'dist/fields.min.json',
'preset_presets': (presetsUrlOverwrite || presetsCdnUrlTemplate) + 'dist/presets.min.json',

'wmf_sitematrix': wmfSitematrixCdnUrl.replace('{version}', '0.1') + 'wikipedia.min.json'
'wmf_sitematrix': wmfSitematrixCdnUrl + 'wikipedia.min.json'
};

let _cachedData = {};
// expose the cache; useful for tests
_this.cache = () => _cachedData;


// Returns a Promise to fetch data
// (resolved with the data if we have it already)
_this.get = (which) => {
Expand Down
5 changes: 4 additions & 1 deletion modules/core/localizer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import { fileFetcher } from './file_fetcher';
import { utilDetect } from '../util/detect';
import { utilStringQs } from '../util';
import { utilArrayUniq } from '../util/array';
import { presetsCdnUrl } from '../../config/id.js';
import { presetsCdnUrlTemplate } from '../../config/id.js';
import packageJSON from '../../package.json';

let _mainLocalizer = coreLocalizer(); // singleton
let _t = _mainLocalizer.t;
Expand Down Expand Up @@ -88,6 +89,8 @@ export function coreLocalizer() {
'locales' // load the list of supported locales
];

const presetsVersion = packageJSON.devDependencies['@openstreetmap/id-tagging-schema'];
const presetsCdnUrl = presetsCdnUrlTemplate.replace('{presets_version}', presetsVersion);
Comment on lines +92 to +93
Copy link
Member

Choose a reason for hiding this comment

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

actually… this was working as intended:

fileFetcher.get expands the "template" with the correct version of the loaded presets. Just using the version from packageJson doesn't always work because of caching: say you have v5.0.1 of one of the presets' files cached, but the rest is loaded fresh from the cdn (say, v5.2.1), then these files might very likely mismatch in at least a few places, which can result in missing translations for example.

To avoid this, the fileFetcher does first load the preset's package.json and use the exact version it gets from that request to load all further preset files. This assures that all files are consistent with each other.

--

The url template can be passed to the fileFetcher:

        const localeDirs = {
            general: 'locales',
            tagging: presetsCdnUrlTemplate + 'dist/translations'
        };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tyrasd even thought this PR is closed, I think your feedback and the fact that I was not able to read those details from the variable names hints that the variable names could be improved. The name itself you communicate if its a "template" – as in, something of the string will need to be replaced in order to work – or a URL that will be used as is.

This might be something to refactor at some point…

const localeDirs = {
general: 'locales',
tagging: presetsCdnUrl + 'dist/translations'
Expand Down
4 changes: 2 additions & 2 deletions modules/services/nsi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import parseVersion from 'vparse';
import { fileFetcher, locationManager } from '../core';
import { presetManager } from '../presets';

import { nsiCdnUrl } from '../../config/id.js';
import { nsiCdnUrlTemplate } from '../../config/id.js';

// Make very sure this resolves to iD's `package.json`
// If you mess up the `../`s, the resolver may import another random package.json from somewhere else.
Expand Down Expand Up @@ -50,7 +50,7 @@ function setNsiSources() {
const nsiVersion = packageJSON.dependencies['name-suggestion-index'] || packageJSON.devDependencies['name-suggestion-index'];
const v = parseVersion(nsiVersion);
const vMinor = `${v.major}.${v.minor}`;
const cdn = nsiCdnUrl.replace('{version}', vMinor);
const cdn = nsiCdnUrlTemplate.replace('{version}', vMinor);
const sources = {
'nsi_data': cdn + 'dist/nsi.min.json',
'nsi_dissolved': cdn + 'dist/dissolved.min.json',
Expand Down