Add Jest tests and publish automation with changesets #10

robmosca · 2023-10-12T10:05:03Z

This PR addresses issues:

In particular:

I added Jest tests covering 100% of the code
I included automation for running Jest tests
I included a command for type checking with Typescript and fixed a minor type problem with the window object
I included changesets
I included automation of deployments through changesets and the changesets release action for GitHub

For this 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 maintainers create an NPM_TOKEN secret with an NPM token with write permissions to the organization (or at least the corresponding package, for publishing).

See single-spa/single-spa-react#178 for a description of how this works.

The automated pipeline includes:

running the tests
checking the format
checking the types
running the linter

Current coverage:

robmosca · 2023-10-12T10:06:10Z

src/import-map-injector.ts

@@ -31,7 +31,7 @@ injectorImportMaps.forEach((scriptEl) => {
            return r.json();
          } else {
            throw Error(
-              `${errPrefix} import map at url '${scriptEl.src}' must respond with a success HTTP status, but responded with HTTP ${r.status} ${r.statusText}`,
+              `${errPrefix} Import map at url '${scriptEl.src}' must respond with a success HTTP status, but responded with HTTP ${r.status} ${r.statusText}`,


To harmonize the casing to the other error messages

robmosca · 2023-10-12T10:06:21Z

src/import-map-injector.ts

@@ -69,8 +69,12 @@ const requiresMicroTick = importMapJsons.some(
  (json) => json instanceof Promise,
 );

+const globalWindow = window as typeof window & {


Needed by Typescript (otherwise throws a type error)

robmosca · 2023-10-12T10:06:32Z

src/import-map-injector.ts

@@ -112,3 +116,5 @@ function injectImportMap(importMaps: ImportMap[]): void {
  finalImportMapScriptEl.textContent = JSON.stringify(finalImportMap);
  document.head.appendChild(finalImportMapScriptEl);
 }
+
+export {};


Needed in order to be able to import the module in Jest. Otherwise, it complains the file is not a module.

joeldenning

Is it possible to make changesets automatically publish without creating another PR? I don't want changeset-bot to spam my Github notifications.

robmosca · 2023-10-20T07:33:44Z

Is it possible to make changesets automatically publish without creating another PR? I don't want changeset-bot to spam my Github notifications.

I don’t know if it is possible to publish automatically when merging, I can investigate, but I would recommend against it.
Having the new PR being opened allows us to have some time to really check that the changes are OK and also allows to avoid republishing every minor change that we merge to main, making our releases more meaningful.
It is just one PR, not that many, and the only thing one has to do is to merge that PR when ready to publish. I wouldn’t consider that too much of a hassle, considering the advantage it gives in controlling your publishing strategy.

joeldenning · 2023-10-24T23:13:56Z

I don’t know if it is possible to publish automatically when merging, I can investigate, but I would recommend against it.

I have significantly more Github notifications than I can manage already. Creating a new PR after every merged pull request will result in an extra notification every time I approve and merge anything. We should look into whether it can be done without an extra pull request. I never asked for autopublishes to be added to all the repos, and it has caused problems in other repos already. If I get more time later, I'll look into the changeset documentation to see if we can avoid adding busy work merging another PR to my Github queue.

robmosca · 2023-10-25T10:49:23Z

I don’t know if it is possible to publish automatically when merging, I can investigate, but I would recommend against it.

I have significantly more Github notifications than I can manage already. Creating a new PR after every merged pull request will result in an extra notification every time I approve and merge anything. We should look into whether it can be done without an extra pull request. I never asked for autopublishes to be added to all the repos, and it has caused problems in other repos already. If I get more time later, I'll look into the changeset documentation to see if we can avoid adding busy work merging another PR to my Github queue.

It was not a PR for every merged pull request. It's just one PR for release, that is updated every time a new merge to main happens. This is a well-established flow defined by changeset.
In any case, closing this PR.

robmosca added 2 commits September 11, 2023 16:54

Add Jest tests and publish automation with changesets

8cca174

Update changeset config and pnpm version

f90fdbd

robmosca changed the title ~~Issue 4 automate tests~~ Add Jest tests and publish automation with changesets Oct 12, 2023

robmosca commented Oct 12, 2023

View reviewed changes

robmosca mentioned this pull request Oct 12, 2023

Add Jest tests and publish automation with changesets #9

Closed

robmosca requested a review from joeldenning October 12, 2023 10:07

joeldenning reviewed Oct 19, 2023

View reviewed changes

robmosca requested a review from joeldenning October 20, 2023 07:33

robmosca closed this Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Jest tests and publish automation with changesets #10

Add Jest tests and publish automation with changesets #10

robmosca commented Oct 12, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

robmosca Oct 12, 2023

joeldenning left a comment

robmosca commented Oct 20, 2023

joeldenning commented Oct 24, 2023 •

edited

Loading

robmosca commented Oct 25, 2023 •

edited

Loading

Add Jest tests and publish automation with changesets #10

Add Jest tests and publish automation with changesets #10

Conversation

robmosca commented Oct 12, 2023

robmosca Oct 12, 2023

Choose a reason for hiding this comment

robmosca Oct 12, 2023

Choose a reason for hiding this comment

robmosca Oct 12, 2023

Choose a reason for hiding this comment

joeldenning left a comment

Choose a reason for hiding this comment

robmosca commented Oct 20, 2023

joeldenning commented Oct 24, 2023 • edited Loading

robmosca commented Oct 25, 2023 • edited Loading

joeldenning commented Oct 24, 2023 •

edited

Loading

robmosca commented Oct 25, 2023 •

edited

Loading