Introduce the hash URL param presetUrlOverwrite
#9475
Conversation
- Use explicit "template" name in variable which need to be replaced. - Apply the replace in `coreLocalizer()` as well
…ma distributions … without a new iD Editor release.
|
Side note: The commit 1 could be sherry picked in main even if the other part is not merged. I find it quite a bit clearer and I also wonder if there is a bug in main ATM, since in one instance the cdn-url was not parsed before using it… |
tyrasd
added
api
There was a problem hiding this comment.
Thanks for this!
See a few remarks below:
- I'd rename the hash parameter to
presetsUrlfor simplicity - There is some special logic for handling the
{presets_version}, see below the comment inlocalizer.js
On a more general note, I'm currently slightly hesitant to allow loading data from arbitrary URLs via a user-supplied parameter. This is because in the aftermath of #8813 I had to fix a gazillion of potential html injection / xss attack vectors, some of which were directly related with strings from presets' translations. While I think that I filled all holes where strings are supplied from the outside (e.g. come in through OSM tags via the OSM API, or other external services), I might not have been that thorough where it comes to translated strings originating from transifex.
So, before merging this branch, I will perform a review of the relevant parts of the code where these presets' strings are rendered. That might take some extra time, though.
| const presetsVersion = packageJSON.devDependencies['@openstreetmap/id-tagging-schema']; | ||
| const presetsCdnUrl = presetsCdnUrlTemplate.replace('{presets_version}', presetsVersion); |
There was a problem hiding this comment.
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'
};There was a problem hiding this comment.
@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…
| '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', |
There was a problem hiding this comment.
These files (except presets_package) should use the presetsCdnUrlTemplate instead of presetsCdnUrlTemplate
There was a problem hiding this comment.
For example:
| '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', |
| @@ -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`. | |||
There was a problem hiding this comment.
| * __`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. |
| 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); | ||
| } |
There was a problem hiding this comment.
| 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); |
|
with openstreetmap/id-tagging-schema#800, this is not needed anymore, as a dedicated customized instance of iD for each PR branch will be built alongside the presets to be previewed |
This enables us to create branch deployments that run
npm run distin theid-editor-schema repo and preview the schema using an URL like
http://preview.ideditor.com/master/#locale=en&presetUrlOverwrote=https://branch-preview-123.netlify.app/.One Issue I found was, that translations don't work well. That might be due to the way I ran
npm run distmy id-tagging-schema folder locally. Or maybe some piece is missing. One thing we will likely need to do is force the locale toenvia URL (as I did in the example above) so the fallback values from the dist-folder are applied.This is the first piece to solve openstreetmap/id-tagging-schema#289. The other part would be to setup netlify with branch deployments in the id-editor-schema repo. — Update: My comment openstreetmap/id-tagging-schema#289 (comment) outlines who I would set that up.