Skip to content

remove pointless dependency#11644

Merged
tyrasd merged 1 commit intodevelopfrom
kh/vparse
Mar 4, 2026
Merged

remove pointless dependency#11644
tyrasd merged 1 commit intodevelopfrom
kh/vparse

Conversation

@k-yle
Copy link
Copy Markdown
Collaborator

@k-yle k-yle commented Dec 5, 2025

When constructing a CDN URL for NSI assets, NSI's version number is parsed with "vparse", and then the semver-patch part is removed.

As far as I can tell, this extra parsing is not required, because the jsdeliver CDN supports any semver syntax. None of the other assets do this extra parsing.

cc @bhousel, since this you added this because of #8305 (comment)

@bhousel
Copy link
Copy Markdown
Member

bhousel commented Dec 5, 2025

Yeah this is fine, good suggestion @k-yle 👍

I remember adding this in 8db1c1f mostly because Milos asked for it here #8305 (comment)

I remember being weakly against the idea of having code that automatically references the dependencies in package.json, but I didn't really have a strong opinion and didn't want to hold up the PR, so I added it.

My opinion is still that urls like these should be updated manually (by a human) and with intention.

@k-yle k-yle added the chore Improvements to the iD development experience or codebase label Jan 14, 2026
Comment thread package-lock.json Outdated
Comment thread modules/services/nsi.js
const v = parseVersion(nsiVersion);
const vMinor = `${v.major}.${v.minor}`;
const cdn = nsiCdnUrl.replace('{version}', vMinor);
const cdn = nsiCdnUrl.replace('{version}', nsiVersion);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am trying to confirm that my test plan makes sense: it is executed at runtime, when user starts editing, right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes, correct. We can check the browser's devtools to see if the correct version of NSI is loaded:

Untitled

The released version of iD currently uses @7.0, this PR will change it to @~7.0 to exactly match the value in package.json

Copy link
Copy Markdown
Contributor

@matkoniecz matkoniecz left a comment

Choose a reason for hiding this comment

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

NSI continues to work in my test

@tyrasd tyrasd merged commit 714d5d4 into develop Mar 4, 2026
4 checks passed
@k-yle k-yle deleted the kh/vparse branch March 5, 2026 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Improvements to the iD development experience or codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants