[MAJOR] Expand relative imports in external maps #81

robmosca · 2023-06-18T19:54:28Z

Fixes issues #80.

In order to fix the issue, I propose expanding relative URLs in external maps when they are loaded. In this way those mappings:

will be displayed with a correct "domain" value when shown in the UI
will be treated correctly when introducing devLibs overrides (otherwise they would become relative to the baseUrl of index.html, instead of the URL of the import map, as specified in the import-map specs, here)

This is potentially a breaking change so it would need a major release update.

robmosca · 2023-06-18T19:59:06Z

package.json

@@ -61,5 +61,6 @@
  },
  "dependencies": {
    "cookie": "^0.4.1"
-  }
+  },
+  "packageManager": "[email protected]"


Updated the pnpm-lock.yaml file to a more recent version of pnpm. I suggest adding this line to make explicit which version of pnpm goes well with the existing lock file.

robmosca · 2023-06-18T19:59:37Z

.gitignore

@@ -63,3 +63,6 @@ dist

 # jetbrain IDE files
 /.idea
+
+# VSCode IDE files
+.vscode


To prevent accidentally adding VSCode settings

robmosca · 2023-06-18T20:00:00Z

src/api/js-api.js

+              return Promise.all([promise, nextPromise]).then(
+                ([originalMap, newMap]) =>
+                  imo.mergeImportMap(originalMap, newMap)


Prettier seems to be reformatting some lines...

I cannot explain this change from prettier. I verified I am using the version of prettier pinned in the dependencies.

robmosca · 2023-06-18T20:00:21Z

src/api/js-api.js

-        console.warn(
-          Error(`Unable to download external import map at url '${url}'`)
-        );
-        imo.invalidExternalMaps.push(new URL(url, document.baseURI).href);
-        return createEmptyImportMap();
-      }
-    );
+      )
+      .then((importMap) => expandRelativeUrlsInImportMap(importMap, url));


This is the actual change

src/api/js-api.js

robmosca · 2023-06-18T20:09:04Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "import-map-overrides",
-  "version": "3.0.0",
+  "version": "4.0.0",


This is potentially a breaking change. What do you think @joeldenning?

joeldenning

This looks good. Were you able to test it very much? I'm thinking this might not need to be a major/breaking change to import-map-overrides, since it only impacts how the URLs are displayed in the UI. Do you agree?

joeldenning · 2023-06-30T22:07:31Z

src/api/js-api.js

+    const outUrl = new URL(url, baseUrl);
+    return outUrl.href;
+  } catch (err) {
+    return url;


Is returning the relative url the correct thing to do when there is an error? The error would only happen if baseUrl is wrong, right? I'm not sure what the best UX is for when the baseUrl is invalid

Yeah, me neither. I wasn't sure so I thought that returning the relative url would still be a reasonable thing to do. I am open to change it as you see fit, if there is a better way of handling this. I don't think this should ever happen, actually.

joeldenning · 2023-06-30T22:09:00Z

src/api/js-api.js

          );
-          imo.invalidExternalMaps.push(r.url);
+          imo.invalidExternalMaps.push(new URL(url, document.baseURI).href);


👍 this is a good change

Actually, this line was already there (line 487 in the older version). It looks like a change because of the modifications from prettier. See this comment about it.

robmosca · 2023-07-01T17:00:06Z

src/ui/list/list.component.js

+        externalOverrideModules.push({
+          ...mod,
+          overrideUrl: this.state.currentPageMap.imports[moduleName],
+        });


When including an external override the domain is displayed from the defaultMap, not from the overridden entry.

robmosca · 2023-07-01T17:03:30Z

This looks good. Were you able to test it very much? I'm thinking this might not need to be a major/breaking change to import-map-overrides, since it only impacts how the URLs are displayed in the UI. Do you agree?

I tested it in the following scenarios:

Importing dependencies from an external import map (including both normal dependencies and dependencies that can be submit to dev overrides)
Adding an override from an external import map.

In this second scenario, I realised there was still a problem with the visualisation of the domain, which I tried to fix here).
Also, in our company we are using a version corresponding to this PR and, at the moment, it seems to be working correctly.

As for the version change. This PR will change the way modified dev dependencies will be loaded. The import-map-overrides changes automatically the production version of some dependencies to the development counterpart. When it does that, the overridden dependencies are injected in an import map after all the ones already existing in the document. If those dependencies were specified with a relative path and they were set in an external import map, they were resolved based to the URL of the import map file and when the override happened, they would instead be resolved to the base URL of the index.html file. With this PR, they continue to be resolved w.r.t. the URL of the import map, which might change the behaviour for some existing instances.
It is quite an edge case and the change actually aligns the current implementation to the import map standard, so... yes, it could also be fair to use a patch update (even if, in principle it could break some existing use cases).

As you prefer, Joel, both major or patch are OK for me.

robmosca · 2023-07-01T17:19:57Z

package.json

@@ -10,6 +10,7 @@
    "build": "pnpm run clean && cross-env NODE_ENV=production rollup -c",
    "build:dev": "pnpm run clean && cross-env NODE_ENV=development rollup -c",
    "clean": "rimraf dist",
+    "check-format": "prettier --check src",


I added this command to check the format with prettier. I am not sure why so many changes from prettier. I am using the version from the node_modules folder as you can see in this image:

🤷🏽‍♂️

robmosca force-pushed the fix-remote-relative-dev-libs-imports branch from acf8be5 to 9ed3f66 Compare June 18, 2023 19:57

robmosca commented Jun 18, 2023

View reviewed changes

src/api/js-api.js Show resolved Hide resolved

robmosca force-pushed the fix-remote-relative-dev-libs-imports branch from 9ed3f66 to 609fd2c Compare June 18, 2023 20:08

robmosca commented Jun 18, 2023

View reviewed changes

robmosca force-pushed the fix-remote-relative-dev-libs-imports branch from 609fd2c to 23b1ed8 Compare June 29, 2023 16:04

robmosca changed the title ~~Expand relative imports in external maps~~ [MAJOR] Expand relative imports in external maps Jun 29, 2023

joeldenning approved these changes Jun 30, 2023

View reviewed changes

robmosca and others added 2 commits July 1, 2023 18:56

Expand relative imports in external maps

4e4b1e6

Fix display of domain for external overridden modules

e93d8ff

robmosca force-pushed the fix-remote-relative-dev-libs-imports branch from b07d322 to e93d8ff Compare July 1, 2023 16:56

robmosca commented Jul 1, 2023

View reviewed changes

Add command to check format with prettier

cd81920

robmosca commented Jul 1, 2023

View reviewed changes

TheMcMurder merged commit 4e90b5d into single-spa:main Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAJOR] Expand relative imports in external maps #81

[MAJOR] Expand relative imports in external maps #81

robmosca commented Jun 18, 2023 •

edited

Loading

robmosca Jun 18, 2023

robmosca Jun 18, 2023

robmosca Jun 18, 2023

robmosca Jul 1, 2023 •

edited

Loading

robmosca Jun 18, 2023

robmosca Jun 18, 2023

joeldenning left a comment

joeldenning Jun 30, 2023

robmosca Jul 1, 2023

joeldenning Jun 30, 2023

robmosca Jul 1, 2023

robmosca Jul 1, 2023

robmosca commented Jul 1, 2023 •

edited

Loading

robmosca Jul 1, 2023 •

edited

Loading

[MAJOR] Expand relative imports in external maps #81

[MAJOR] Expand relative imports in external maps #81

Conversation

robmosca commented Jun 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robmosca Jul 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robmosca commented Jul 1, 2023 • edited Loading

robmosca Jul 1, 2023 • edited Loading

Choose a reason for hiding this comment

robmosca commented Jun 18, 2023 •

edited

Loading

robmosca Jul 1, 2023 •

edited

Loading

robmosca commented Jul 1, 2023 •

edited

Loading

robmosca Jul 1, 2023 •

edited

Loading