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

Introduce the hash URL param presetUrlOverwrite #9475

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions API.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ of iD (e.g. `https://ideditor-release.netlify.app`), the following parameters ar
* __`validationError`__ - The issues identified by these types/subtypes will be treated as errors (i.e. Issues will be surfaced to the user but will block changeset upload). Each parameter value should contain a urlencoded, comma-separated list of type/subtype match rules. An asterisk `*` may be used as a wildcard.<br/>
_Example:_ `validationError=crossing_ways/highway*,crossing_ways/tunnel*`
* __`walkthrough=true`__ - Start the walkthrough automatically
* __`presetUrlOverwrite=https://example.com`__ - Overwrite the source for presets. Default is the [id-tagging-schema](https://github.com/openstreetmap/id-tagging-schema) release on jsdelivery ([Code](https://github.com/openstreetmap/iD/blob/develop/config/id.js#L2)). The URL has to serve the root of the id-tagging-schema repo after running `npm run dist`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* __`presetUrlOverwrite=https://example.com`__ - Overwrite the source for presets. Default is the [id-tagging-schema](https://github.com/openstreetmap/id-tagging-schema) release on jsdelivery ([Code](https://github.com/openstreetmap/iD/blob/develop/config/id.js#L2)). The URL has to serve the root of the id-tagging-schema repo after running `npm run dist`.
* __`presetsUrl=https://example.com/`__ - Overwrites the source for presets. Default is the [id-tagging-schema](https://github.com/openstreetmap/id-tagging-schema) release on jsDelivr ([code](https://github.com/openstreetmap/iD/blob/develop/config/id.js#L2)). The URL has to serve the root of the id-tagging-schema repo after running `npm run dist` and allow [CORS](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS) requests.


##### iD on openstreetmap.org (Rails Port)

Expand Down
16 changes: 8 additions & 8 deletions config/id.js
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
27 changes: 19 additions & 8 deletions modules/core/file_fetcher.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import parseVersion from 'vparse';
import { presetsCdnUrl, ociCdnUrl, wmfSitematrixCdnUrl } from '../../config/id.js';
import { presetsCdnUrlTemplate, ociCdnUrlTemplate, wmfSitematrixCdnUrlTemplate } from '../../config/id.js';
import { utilStringQs } from '../util';

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

Expand All @@ -11,10 +12,21 @@ export { _mainFileFetcher as fileFetcher };
// coreFileFetcher asynchronously fetches data from JSON files
//
export function coreFileFetcher() {
const presetsVersion = packageJSON.devDependencies['@openstreetmap/id-tagging-schema'];
let presetsCdnUrl = presetsCdnUrlTemplate.replace('{presets_version}', presetsVersion);
// Allow to overwrite the preset source to enable testing PRs for the id-editor-presets repo.
const presetUrlOverwrite = utilStringQs(window.location.hash)?.presetUrlOverwrote;
if (presetUrlOverwrite) {
presetsCdnUrl = presetUrlOverwrite;
console.info('`presetUrlOverwrite` applied. Presets are fetched from', presetsCdnUrl);
}
Comment on lines +16 to +22
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let presetsCdnUrl = presetsCdnUrlTemplate.replace('{presets_version}', presetsVersion);
// Allow to overwrite the preset source to enable testing PRs for the id-editor-presets repo.
const presetUrlOverwrite = utilStringQs(window.location.hash)?.presetUrlOverwrote;
if (presetUrlOverwrite) {
presetsCdnUrl = presetUrlOverwrite;
console.info('`presetUrlOverwrite` applied. Presets are fetched from', presetsCdnUrl);
}
// Allow to overwrite the presets source to enable testing PRs for the id-editor-presets repo.
const presetsUrlOverwrite = utilStringQs(window.location.hash)?.presetsUrl;
const presetsCdnUrl = presetsUrlOverwrite || 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 +41,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

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
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
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