Add tests and automated publishing the package to NPM #96

robmosca · 2023-09-29T07:40:14Z

This PR:

introduces automated publishing of the package using the changeset action. The setup is identical to the one in single-spa-react
introduces Jest tests on the API

For automated publishing to work, it is necessary that:

The GITHUB_TOKEN is granted permissions to create and approve pull requests. This can be done in Settings -> Actions -> General -> Workflow permissions -> Allow GitHub Actions to create and approve pull requests
The admin of the repo creates an NPM_TOKEN secret with an NPM token with write permissions to the organization (or at least the corresponding package, for publishing).

changeset-bot · 2023-09-29T07:40:17Z

🦋 Changeset detected

Latest commit: fd32330

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
import-map-overrides	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

joeldenning

Thanks for your work on this, these tests will help a lot. I've left a few comments and questions

Do I need to add an NPM_TOKEN environment variable to this repo before merging it?

joeldenning · 2023-10-11T22:16:00Z

.changeset/config.json

+  "$schema": "https://unpkg.com/@changesets/[email protected]/schema.json",
+  "changelog": [
+    "@changesets/changelog-github",
+    { "repo": "single-spa/single-spa-react" }


Suggested change

{ "repo": "single-spa/single-spa-react" }

{ "repo": "single-spa/import-map-overrides }

Sorry about this. Fixed.

joeldenning · 2023-10-11T22:16:49Z

.changeset/plenty-nails-lie.md

+"import-map-overrides": patch
+---
+
+Add automation to publish package


This seems redundant to automate release flow in the fast-chairs-guess.md changeset. Let's remove this changeset?

Of course. This PR has been reworked so many times that with iterations (it has been reopened quite a few times) some comments went lost and some inconsistencies crept in. Removing this file.

.github/workflows/build-test-release.yml

.gitignore

joeldenning · 2023-10-11T22:20:51Z

rollup.config.js

@@ -70,6 +70,7 @@ export default [
    },
    plugins: [
      babel({
+        babelHelpers: "bundled",


Why is this needed? Are we using generator syntax or something else that requires babel runtime?

Without this, when building we get the following warning:

src/import-map-overrides-server.js → dist/import-map-overrides-server.js... babelHelpers: 'bundled' option was used by default. It is recommended to configure this option explicitly, read more here: https://github.com/rollup/plugins/tree/master/packages/babel#babelhelpers

joeldenning · 2023-10-11T22:27:10Z

src/api/js-api.js

-      return promise.then(() =>
-        includes(imo.invalidExternalMaps, importMapUrl)
+      return promise.then(
+        () => !includes(imo.invalidExternalMaps, importMapUrl)


Is this a bug that you fixed? Please link issue if there was one for this

This is indeed a bug I fixed. I discovered it when writing the tests.
The function is called isExternalMapValid and should return true when the map IS valid, which means it is NOT included in the imo.invalidExternalMaps array.

There was no issue about this. The power of unit tests 🙂

joeldenning · 2023-10-11T22:27:19Z

src/api/js-api.js

@@ -367,6 +373,7 @@ function init() {

  function fireEvent(type) {
    // Set timeout so that event fires after the change has totally finished
+


Suggested change

joeldenning · 2023-10-11T22:27:58Z

src/api/localStorageMock.js

@@ -0,0 +1,25 @@
+// Mocks for the localStorage


Seems redundant to have local-storage-mock.js and localStorageMock.js

Another left over from the many reopenings of this PR. Sorry about this. Removed.

joeldenning · 2023-10-11T22:28:17Z

src/server/server-api.js

@@ -1,4 +1,4 @@
-import cookie from "cookie";
+const cookie = require("cookie");


Why the change to CJS?

If we use the ES syntax with the new version of cookie library we get the following error when building with rollup:

src/import-map-overrides-server.js → dist/import-map-overrides-server.js... (!) Unresolved dependencies https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency cookie (imported by src/server/server-api.js)

I am hesitant about this change because it could impact a lot of things (bundling, tests, linter, etc). All ESM is better.

I read through the rollup documentation linked to in the warning you shared (https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency) and the solution seems to be to add @rollup/plugin-node-resolve rather than to change it to a CJS import. Is there an error even when rollup node resolve is present?

Have you tested that import-map-overrides works server-side after this change?

src/util/includes.test.js

robmosca

Addressed the comments.

robmosca · 2023-10-12T09:04:42Z

.changeset/config.json

+  "$schema": "https://unpkg.com/@changesets/[email protected]/schema.json",
+  "changelog": [
+    "@changesets/changelog-github",
+    { "repo": "single-spa/single-spa-react" }


Sorry about this. Fixed.

robmosca · 2023-10-12T09:04:53Z

.changeset/plenty-nails-lie.md

+"import-map-overrides": patch
+---
+
+Add automation to publish package


Of course. This PR has been reworked so many times that with iterations (it has been reopened quite a few times) some comments went lost and some inconsistencies crept in. Removing this file.

.gitignore

.github/workflows/build-test-release.yml

robmosca · 2023-10-12T09:13:20Z

rollup.config.js

@@ -70,6 +70,7 @@ export default [
    },
    plugins: [
      babel({
+        babelHelpers: "bundled",


Without this, when building we get the following warning:

src/import-map-overrides-server.js → dist/import-map-overrides-server.js... babelHelpers: 'bundled' option was used by default. It is recommended to configure this option explicitly, read more here: https://github.com/rollup/plugins/tree/master/packages/babel#babelhelpers

robmosca · 2023-10-12T09:34:08Z

src/api/js-api.js

+                let parsedContent;
+                try {
+                  parsedContent = JSON.parse(scriptEl.textContent);
+                } catch (e) {


I introduced this when I wrote a test and I realized that, if we have an empty import map of the form:

<script type="importmap"></script>

this will make the parsing fail. I considered this would be an unwanted behaviour, hence the change. I understand your comment and I have now reverted this change and opened an issue to further discuss this and maybe reintroduce this in a separate PR.
#98

robmosca · 2023-10-12T09:40:09Z

src/api/js-api.js

+                let parsedContent;
+                try {
+                  parsedContent = JSON.parse(scriptEl.textContent);
+                } catch (e) {


I also modified the tests accordingly.

robmosca · 2023-10-12T09:41:04Z

src/api/js-api.js

-      return promise.then(() =>
-        includes(imo.invalidExternalMaps, importMapUrl)
+      return promise.then(
+        () => !includes(imo.invalidExternalMaps, importMapUrl)


This is indeed a bug I fixed. I discovered it when writing the tests.
The function is called isExternalMapValid and should return true when the map IS valid, which means it is NOT included in the imo.invalidExternalMaps array.

robmosca · 2023-10-12T09:41:26Z

src/api/js-api.js

-      return promise.then(() =>
-        includes(imo.invalidExternalMaps, importMapUrl)
+      return promise.then(
+        () => !includes(imo.invalidExternalMaps, importMapUrl)


There was no issue about this. The power of unit tests 🙂

robmosca · 2023-10-12T09:41:54Z

src/api/localStorageMock.js

@@ -0,0 +1,25 @@
+// Mocks for the localStorage


Another left over from the many reopenings of this PR. Sorry about this. Removed.

robmosca · 2023-10-12T09:49:21Z

Thanks for your work on this, these tests will help a lot. I've left a few comments and questions

Do I need to add an NPM_TOKEN environment variable to this repo before merging it?

For automated publishing to work, @joeldenning, before merging it is necessary that:

The GITHUB_TOKEN is granted permissions to create and approve pull requests. This can be done in Settings -> Actions -> General -> Workflow permissions -> Allow GitHub Actions to create and approve pull requests
The admin of the repo creates an NPM_TOKEN secret with an NPM token with write permissions to the organization (or at least the corresponding package, for publishing).

joeldenning

Looks good, one last comment

joeldenning · 2023-10-12T23:13:13Z

src/server/server-api.js

@@ -1,4 +1,4 @@
-import cookie from "cookie";
+const cookie = require("cookie");


I am hesitant about this change because it could impact a lot of things (bundling, tests, linter, etc). All ESM is better.

I read through the rollup documentation linked to in the warning you shared (https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency) and the solution seems to be to add @rollup/plugin-node-resolve rather than to change it to a CJS import. Is there an error even when rollup node resolve is present?

Have you tested that import-map-overrides works server-side after this change?

robmosca · 2023-10-13T07:48:08Z

I read through the rollup documentation linked to in the warning you shared (https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency) and the solution seems to be to add @rollup/plugin-node-resolve rather than to change it to a CJS import. Is there an error even when rollup node resolve is present?

Have you tested that import-map-overrides works server-side after this change?

To unblock this PR I have reverted all changes related to the warning I get when building import-map-overrides-server.js.
I tracked the problem in this new issue and I can tackle it in a separate PR.

robmosca and others added 7 commits September 29, 2023 09:38

Add Jest tests

ec18ce4

Address comments by Joel Denning

1bf4128

Add more tests

615c215

Fix setDocumentAndLoadScript

cc6afd1

Add tests for external maps

6fe2f38

Add automated release workflow

730e9a3

Adapt changeset config

88727a1

robmosca mentioned this pull request Sep 29, 2023

Add tests and automated publishing the package to NPM #89

Closed

robmosca changed the title ~~Add publish automation and jest tests~~ Add tests and automated publishing the package to NPM Sep 29, 2023

This was referenced Sep 29, 2023

Add tests and automated publishing the package to NPM #95

Closed

Add Jest tests for api #87

Closed

Fix access for changeset config

1636b05

robmosca requested a review from joeldenning September 29, 2023 07:56

joeldenning reviewed Oct 11, 2023

View reviewed changes

Address comments

70155ee

robmosca commented Oct 12, 2023

View reviewed changes

Revert addition of an empty line

efdb239

robmosca requested a review from joeldenning October 12, 2023 09:51

joeldenning reviewed Oct 12, 2023

View reviewed changes

Revert fix on build error

fd32330

robmosca requested a review from joeldenning October 13, 2023 07:49

joeldenning approved these changes Oct 18, 2023

View reviewed changes

joeldenning merged commit cd137ce into main Oct 18, 2023
2 checks passed

joeldenning deleted the add-publish-automation-and-jest-tests branch October 18, 2023 23:30

github-actions bot mentioned this pull request Oct 18, 2023

Version Packages #101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests and automated publishing the package to NPM #96

Add tests and automated publishing the package to NPM #96

robmosca commented Sep 29, 2023

changeset-bot bot commented Sep 29, 2023 •

edited

Loading

joeldenning left a comment

joeldenning Oct 11, 2023

robmosca Oct 12, 2023

joeldenning Oct 11, 2023

robmosca Oct 12, 2023

joeldenning Oct 11, 2023

robmosca Oct 12, 2023

joeldenning Oct 11, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

joeldenning Oct 11, 2023

joeldenning Oct 11, 2023

robmosca Oct 12, 2023

joeldenning Oct 11, 2023

robmosca Oct 12, 2023

joeldenning Oct 12, 2023

robmosca left a comment

robmosca Oct 12, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

robmosca commented Oct 12, 2023 •

edited

Loading

joeldenning left a comment

joeldenning Oct 12, 2023

robmosca commented Oct 13, 2023

	{ "repo": "single-spa/single-spa-react" }
	{ "repo": "single-spa/import-map-overrides }

		@@ -367,6 +373,7 @@ function init() {

		function fireEvent(type) {
		// Set timeout so that event fires after the change has totally finished

		@@ -1,4 +1,4 @@
		import cookie from "cookie";
		const cookie = require("cookie");

Add tests and automated publishing the package to NPM #96

Add tests and automated publishing the package to NPM #96

Conversation

robmosca commented Sep 29, 2023

changeset-bot bot commented Sep 29, 2023 • edited Loading

🦋 Changeset detected

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robmosca commented Oct 12, 2023 • edited Loading

joeldenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robmosca commented Oct 13, 2023

changeset-bot bot commented Sep 29, 2023 •

edited

Loading

robmosca commented Oct 12, 2023 •

edited

Loading