Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade typescript to ^4.2.0 and fix lint issues #1156

Merged
merged 45 commits into from
May 13, 2021
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
77f84d2
Upgrade typescript to ^4.2.0 and fix lint issues
PiotrKozlowski Apr 28, 2021
910476d
Upgrade react-scripts to ^4.0.3
PiotrKozlowski Apr 28, 2021
13ab6ad
Add craco and set url: false in css-loader
PiotrKozlowski Apr 28, 2021
03c181a
Fix lint issues
PiotrKozlowski Apr 28, 2021
be76f0b
Fix part of unit tests
PiotrKozlowski Apr 28, 2021
6630820
Fix another part of unit tests
PiotrKozlowski Apr 28, 2021
fd5550d
Handle ServiceWorker in a new way
PiotrKozlowski Apr 29, 2021
004d7c7
Fix ts errors
PiotrKozlowski Apr 29, 2021
d38ceab
Fix new lint errors
PiotrKozlowski Apr 29, 2021
9d12d67
revert this lint fix due to compilation errors - it should be fixed a…
PiotrKozlowski Apr 29, 2021
e2042d9
Move also service-worker.js.map to books folder
PiotrKozlowski Apr 29, 2021
5b77f29
Fix one more lint error and test
PiotrKozlowski Apr 29, 2021
3fb7cb7
Merge branch 'master' into issue-1469
staxly[bot] Apr 29, 2021
947941c
change AnyRoute signature and add assertNonNullableArray util
PiotrKozlowski Apr 30, 2021
fab6a17
Merge branch 'issue-1469' of github.com:openstax/rex-web into issue-1469
PiotrKozlowski Apr 30, 2021
4521533
small fix for test
PiotrKozlowski Apr 30, 2021
118f7b8
Merge branch 'master' into issue-1469
staxly[bot] Apr 30, 2021
3f38210
Merge branch 'master' into issue-1469
staxly[bot] May 3, 2021
86e5457
Do not use any for animationEvent function
PiotrKozlowski May 4, 2021
b58f604
update loadFont helper
PiotrKozlowski May 4, 2021
789ea7f
Update serviceWorker.ts
PiotrKozlowski May 4, 2021
d85a8fd
remove unused workbox plugins
PiotrKozlowski May 4, 2021
4d1fcc2
Revert "remove unused workbox plugins"
PiotrKozlowski May 6, 2021
eed4309
Merge branch 'master' into issue-1469
PiotrKozlowski May 6, 2021
797c68f
Use craco to move service-worker.js to build/books/
PiotrKozlowski May 6, 2021
d8ea5e4
Remove check for process.env.PUBLIC_URL
PiotrKozlowski May 6, 2021
5be0e06
Default value for process.env.PUBLIC_URL
PiotrKozlowski May 6, 2021
b82a166
remove unused workbox plugins
PiotrKozlowski May 4, 2021
814fc2b
Check if PUBLIC_URL is undefined instead of falsy
PiotrKozlowski May 6, 2021
9efe6bb
Merge branch 'master' into issue-1469
staxly[bot] May 7, 2021
5a86df1
Merge branch 'master' into issue-1469
staxly[bot] May 7, 2021
2c584be
Merge branch 'master' into issue-1469
staxly[bot] May 7, 2021
9f15760
Merge branch 'master' into issue-1469
staxly[bot] May 10, 2021
6942aa2
requested changes
PiotrKozlowski May 11, 2021
598605c
Merge branch 'master' into issue-1469
PiotrKozlowski May 11, 2021
6464dba
remove assertNonNullable
PiotrKozlowski May 11, 2021
a3fd8df
Update @openstax/types
PiotrKozlowski May 12, 2021
a74703b
Do not use deprecated matchMedia.add/removeListener methods
PiotrKozlowski May 12, 2021
8499773
add global KeyboardEventConstructor
jarosik10 Apr 30, 2021
e461755
fix lint and tests
jarosik10 Apr 30, 2021
f055aea
smaller fixes
PiotrKozlowski May 12, 2021
2e23eb9
No need to mock navigation state in Home.spec.tsx
PiotrKozlowski May 12, 2021
d31dc68
Add @babel/plugin-proposal-optional-chaining to babel-config.js
PiotrKozlowski May 12, 2021
497bcec
Update node_modules cache
PiotrKozlowski May 12, 2021
5c58dcd
requested changes
PiotrKozlowski May 13, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions craco.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module.exports = {
style: {
css: {
loaderOptions: {
// https://github.com/openstax/unified/issues/1469
url: false,
TomWoodward marked this conversation as resolved.
Show resolved Hide resolved
},
},
},
};
25 changes: 19 additions & 6 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"version": "0.1.0",
"private": true,
"dependencies": {
"@craco/craco": "^6.1.2",
"@formatjs/intl-pluralrules": "^4.0.6",
"@openstax/highlighter": "^1.7.0",
"@openstax/open-search-client": "0.1.0",
Expand All @@ -27,7 +28,7 @@
"react-intl": "^5.12.0",
"react-loadable": "^5.5.0",
"react-redux": "^7.1.0",
"react-scripts": "3.4.4",
"react-scripts": "^4.0.3",
"redux": "^4.0.1",
"redux-sentry-middleware": "^0.2.2",
"reselect": "^4.0.0",
Expand All @@ -36,10 +37,22 @@
"styled-components": "^4.3.2",
"styled-icons": "^8.1.0",
"typesafe-actions": "^4.4.2",
"typescript": "3.9.6",
"typescript": "^4.2.0",
"url": "^0.11.0",
"uuid": "^3.4.0",
"weak-map": "^1.0.5"
"weak-map": "^1.0.5",
"workbox-background-sync": "^5.1.3",
"workbox-broadcast-update": "^5.1.3",
"workbox-cacheable-response": "^5.1.3",
"workbox-core": "^5.1.3",
"workbox-expiration": "^5.1.3",
"workbox-google-analytics": "^5.1.3",
"workbox-navigation-preload": "^5.1.3",
"workbox-precaching": "^5.1.3",
"workbox-range-requests": "^5.1.3",
"workbox-routing": "^5.1.3",
"workbox-strategies": "^5.1.3",
"workbox-streams": "^5.1.3"
Copy link
Member

Choose a reason for hiding this comment

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

i feel like some of these workbox plugins aren't used, are they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it seems like this. I've copied all of them from the template created with yarn create react-app my-app --template cra-template-pwa-typescript and they are not used there either
I've removed them for now and we can check if they work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing them service worker is not being registered. I'll revert this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it is actually not working after reverting. I think it may be related to the change about PUBLIC_URL:

if (!process.env.PUBLIC_URL) {
    return Promise.reject(
      new Error('service worker won\'t work if PUBLIC_URL is not defined.')
    );
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've ended up changing the if statement to if (process.env.PUBLIC_URL === undefined) { and removed these workbox plugins

Btw. process.env.PUBLIC_URL is set to an empty string on rex-web.herokuapp.com

},
"scripts": {
"trust-localhost": "./script/trust-localhost.bash",
Expand All @@ -49,12 +62,12 @@
"lint:css": "stylelint 'src/**/*.tsx'",
"lint:bash": "shellcheck $(find . -type f \\( -iname '*\\.sh' -or -iname '*\\.bash' \\) | grep -v 'node_modules' )",
"prestart": "npm run-script build:css",
"start": "HTTPS=${HTTPS:-true} react-scripts start",
"start": "HTTPS=${HTTPS:-true} craco start",
"start:static": "export REACT_APP_ENV=${REACT_APP_ENV:-test} && npm run-script build && npm run-script prerender && npm run-script server",
"clean": "rm -rf ./build",
"build": "npm run-script build:css && npm run-script build:js && ./script/doctor-workbox.bash",
"build:css": "lessc --source-map --source-map-include-source ./generic-styles/index.less ./src/content.css",
"build:js": "export REACT_APP_ENV=${REACT_APP_ENV:-production} && GENERATE_SOURCEMAP=${GENERATE_SOURCEMAP:-true} react-scripts build",
"build:js": "export REACT_APP_ENV=${REACT_APP_ENV:-production} && GENERATE_SOURCEMAP=${GENERATE_SOURCEMAP:-true} craco build",
"build:clean": "npm run-script clean && npm run-script build",
"prerender": "REACT_APP_ENV=${REACT_APP_ENV:-production} node ./script/entry prerender/index",
"server": "REACT_APP_ENV=${REACT_APP_ENV:-development} node ./script/entry server/cli",
Expand All @@ -76,7 +89,7 @@
"test:prerender:browser": "REACT_APP_ENV=test SERVER_MODE=built jest --testPathPattern=\"(\\.|/)browserspec\\.tsx?\" --config jest-puppeteer.config.json -i",
"test:prerender:screenshots": "REACT_APP_ENV=test SERVER_MODE=built jest --testPathPattern=\"(\\.|/)screenshotspec\\.tsx?\" --config jest-puppeteer.config.json",
"test:lighthouse-manual": "REACT_APP_ENV=test lighthouse --view --config-path=./src/test/audits/index.js",
"analyze:bundle": "react-scripts build && source-map-explorer 'build/static/js/*.js' -m",
"analyze:bundle": "craco build && source-map-explorer 'build/static/js/*.js' -m",
"analyze:dom": "node ./script/entry.js domVisitor",
"heroku-postbuild": "npm run-script build:clean"
},
Expand Down
71 changes: 2 additions & 69 deletions script/doctor-workbox.bash
Original file line number Diff line number Diff line change
Expand Up @@ -3,82 +3,15 @@
# sed expression needs $s
TomWoodward marked this conversation as resolved.
Show resolved Hide resolved
set -e

echo "HACKING WORKBOX SCRIPT WITH STRING REPLACEMENT"
echo "remove this when offical solution is merged https://github.com/facebook/create-react-app/pull/5369"

function abs_path {
(cd "$(dirname '$1')" &>/dev/null && printf "%s/%s" "$PWD" "${1##*/}")
}

build=$(abs_path "${BASH_SOURCE%/*}/../build")
worker="$build"/service-worker.js

# add cache behaviors for 100% offline load
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved it to src/service-worker.ts

cp "$worker" "$worker".bak
head -n 39 "$worker".bak > "$worker"

cat >> "$worker" <<- EOM
workbox.routing.registerRoute(
/https:\/\/buyprint\.openstax\.org/,
new workbox.strategies.StaleWhileRevalidate({ // can't use cachefirst because responses are opaque
cacheName: 'buy-print',
plugins: [
new workbox.expiration.Plugin({
maxEntries: 20,
}),
],
})
);

workbox.routing.registerRoute(
/https:\/\/cdnjs\.cloudflare\.com\/ajax\/libs\/mathjax\//,
new workbox.strategies.StaleWhileRevalidate({ // can't use cachefirst because responses are opaque
cacheName: 'cdn-assets',
plugins: [
new workbox.expiration.Plugin({
maxEntries: 100, // mathjax loads a lot of files
}),
],
})
);

workbox.routing.registerRoute(
/\/cms\/assets\//,
new workbox.strategies.StaleWhileRevalidate({
cacheName: 'cms-assets',
plugins: [
new workbox.expiration.Plugin({
maxEntries: 20,
}),
],
})
);

workbox.routing.registerRoute(
/\/contents|resources|extras\//,
new workbox.strategies.CacheFirst({
cacheName: 'book-content',
plugins: [
new workbox.expiration.Plugin({
maxEntries: 300,
}),
],
})
);

workbox.routing.registerRoute(
/\/apps\/cms\/api\//,
new workbox.strategies.StaleWhileRevalidate({
cacheName: 'cms-api',
plugins: [
new workbox.expiration.Plugin({
maxEntries: 20,
}),
],
})
);
EOM
workerMap="$build"/service-worker.js.map

mkdir "$build"/books

mv "$worker" "$build"/books/
mv "$workerMap" "$build"/books/
2 changes: 1 addition & 1 deletion src/app/content/components/BookBanner.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('BookBanner', () => {
resetModules();

window = assertWindow();
delete window.location;
delete (window as any).location;

window.location = {
assign: jest.fn(),
Expand Down
2 changes: 1 addition & 1 deletion src/app/content/components/Page/contentLinkHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export const contentLinkHandler = (anchor: HTMLAnchorElement, getProps: () => Co
return;
}

const base = new URL(assertWindow().location);
const base = new URL(assertWindow().location.href);
TomWoodward marked this conversation as resolved.
Show resolved Hide resolved
base.hash = '';
base.search = '';

Expand Down
2 changes: 1 addition & 1 deletion src/app/content/components/Page/highlightManager.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('highlightManager', () => {
});

afterEach(() => {
delete window.document.getSelection;
delete (window as any).document.getSelection;
});

it('CardList is rendered initially', () => {
Expand Down
2 changes: 2 additions & 0 deletions src/app/content/components/utils/allImagesLoaded.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ describe('allImagesLoaded', () => {
finished = false;
element = assertDocument().createElement('div');
img = assertDocument().createElement('img');

Object.defineProperty(img, 'complete', { value: false, writable: true });
});

it('resolves when passed an element with no images', async() => {
Expand Down
2 changes: 2 additions & 0 deletions src/app/content/content.prerenderspec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ describe('content', () => {
['[data-testid="toc"]', 'style'],
['[data-testid="search-results-sidebar"]', 'style'],
['[data-testid="loader"] path', 'style'],
// img src is changed from data:image/svg+xml;base64... to static path
['[data-testid="navbar"] img', 'src'],
].forEach(([selector, attribute]) => {
root.querySelectorAll(selector).forEach((element) =>
element.removeAttribute(attribute)
Expand Down
6 changes: 3 additions & 3 deletions src/app/content/highlights/components/Note.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const TextArea = styled.textarea`

// tslint:disable-next-line:variable-name
const Note = ({onChange, onFocus, note, textareaRef}: Props) => {
const setTextAreaHeight = () => {
const setTextAreaHeight = React.useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-hooks/exhaustive-deps error in useEffect below

const element = textareaRef.current;
if (!element) {
return;
Expand All @@ -45,9 +45,9 @@ const Note = ({onChange, onFocus, note, textareaRef}: Props) => {
if (element.scrollHeight > element.offsetHeight) {
element.style.height = `${element.scrollHeight + 5}px`;
}
};
}, [textareaRef]);

React.useEffect(setTextAreaHeight, [note]);
React.useEffect(setTextAreaHeight, [note, setTextAreaHeight]);

return <TextArea
ref={textareaRef}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export const useOnClickOutside = (
cb: (e: MouseEvent) => void,
eventOptions?: EventListenerOptions
) => {
// eslint-disable-next-line react-hooks/exhaustive-deps
TomWoodward marked this conversation as resolved.
Show resolved Hide resolved
React.useEffect(onClickOutside(element, isEnabled, cb, eventOptions), [element, cb, isEnabled]);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ describe('SearchResultsSidebar', () => {

jest.useFakeTimers();
if (sidebar.searchSidebar.current) {
sidebar.searchSidebar.current.dispatchEvent(animationEvent());
sidebar.searchSidebar.current.dispatchEvent(animationEvent() as any);
Copy link
Member

Choose a reason for hiding this comment

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

why is the any needed for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because animationEvent is returning CustomEvent instead of Event. I'll update the function instead using any

Argument of type 'CustomEvent<unknown>' is not assignable to parameter of type 'Event'.
  Types of property 'srcElement' are incompatible.
    Type 'EventTarget | null' is not assignable to type 'Element | null'.
          Type 'EventTarget' is missing the following properties from type 'Element': assignedSlot, attributes, classList, className, and 119 more.ts(2345)
    ```

}
jest.runAllTimers();

Expand Down
4 changes: 2 additions & 2 deletions src/app/content/studyGuides/hooks/printStudyGuides.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ describe('printStudyGuides', () => {
it('waits for promiseCollector.calm', async() => {
let resolveCalm: undefined | (() => void);

calmSpy.mockReturnValue(new Promise((resolve) => {
calmSpy.mockReturnValue(new Promise<void>((resolve) => {
resolveCalm = resolve;
}));

Expand Down Expand Up @@ -125,7 +125,7 @@ describe('printStudyGuides', () => {
it('doesn\'t print if study guides modal was closed', async() => {
let resolveCalm: undefined | (() => void);

calmSpy.mockReturnValue(new Promise((resolve) => {
calmSpy.mockReturnValue(new Promise<void>((resolve) => {
resolveCalm = resolve;
}));

Expand Down
4 changes: 2 additions & 2 deletions src/app/content/utils/urlUtils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('getUrlParamForPageId', () => {
});

it('returns uuid param if tree is missing a slug', () => {
delete book.tree.contents[0].slug;
delete (book as any).tree.contents[0].slug;
TomWoodward marked this conversation as resolved.
Show resolved Hide resolved

expect(getUrlParamForPageId(book, 'pagelongid@1')).toEqual({uuid: 'pagelongid'});
});
Expand All @@ -94,7 +94,7 @@ describe('getUrlParamForPageId', () => {
});

it('throws if tree is missing a slug and env is production', () => {
delete book.tree.contents[0].slug;
delete (book as any).tree.contents[0].slug;
expect(() =>
getUrlParamForPageId(book, 'pagelongid@1')
).toThrowErrorMatchingInlineSnapshot(
Expand Down
2 changes: 1 addition & 1 deletion src/app/developer/components/Home.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ describe('Home', () => {

it('matches snapshot', async() => {
jest.spyOn(Date.prototype, 'getFullYear').mockReturnValue(2021);
const store = createTestStore({navigation: new URL('https://localhost')});
const store = createTestStore({navigation: new URL('https://localhost') as any});
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type 'URL' is not assignable to type 'State'.
  Property 'state' is missing in type 'URL' but required in type 'Location<any>'.ts(2322)

I guess this didn't change recently but now it is an error so it should be okay to just use any

Copy link
Member

@TomWoodward TomWoodward May 11, 2021

Choose a reason for hiding this comment

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

you could cast to Location instead, maybe Location<undefined>, or combine it with {state: {}} if state is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to new URL('https://localhost') as any as State

Copy link
Member

Choose a reason for hiding this comment

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

if url legitimately doesn't have a field that state requires, it would probably be better to add that field by doing

{...new URL('https://localhost'), state: {}}

or whatever

const component = renderer.create(<Provider store={store}>
<Services.Provider value={services}>
<MessageProvider>
Expand Down
2 changes: 1 addition & 1 deletion src/app/developer/components/Routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { H3 } from '../../components/Typography';
import Panel from './Panel';

// tslint:disable-next-line:variable-name
const Routes: React.SFC = () => <Panel title='Routes'>
const Routes = () => <Panel title='Routes'>
{routes.map((route) => <div key={route.name}>
<H3>{route.name}</H3>
path: {route.paths.map((path) => <React.Fragment key={path}>{path}<br /></React.Fragment>)}
Expand Down
2 changes: 1 addition & 1 deletion src/app/head/hooks/locationChange.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { assertNotNull } from '../../utils';
describe('setHead hook', () => {
let hookBody: ActionHookBody<typeof locationChange>;
const helpers = {} as MiddlewareAPI & AppServices;
const action = locationChange({location: new URL('http://localhost/'), action: 'PUSH'});
const action = locationChange({location: new URL('http://localhost/') as any, action: 'PUSH'});
Copy link
Member

Choose a reason for hiding this comment

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

same deal with the location/url situation


beforeEach(() => {
resetModules();
Expand Down
2 changes: 1 addition & 1 deletion src/app/index.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ describe('create app', () => {
pathname: notFound.getUrl({url: 'url'}),
replace: jest.fn(),
};
delete window.location;
delete (window as any).location;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a fix for The operand of a 'delete' operator must be optional.. Is it something we want to intentionally ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so

window.location = newLocation;

createApp = require('./index').default;
Expand Down
5 changes: 3 additions & 2 deletions src/app/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { matchPathname } from './navigation/utils';
import * as notifications from './notifications';
import createReducer from './reducer';
import { AppServices, AppState, Middleware } from './types';
import { assertNonNullableArray } from './utils/assertions';

export const actions = {
app: appAactions,
Expand All @@ -32,15 +33,15 @@ export const actions = {
notifications: notifications.actions,
};

export const routes = Object.values({
export const routes = assertNonNullableArray(Object.values({
...(
process.env.REACT_APP_ENV !== 'production'
? developer.routes
: /* istanbul ignore next */ {}
),
...content.routes,
...errors.routes,
});
}), 'some of the required routes are not exported');
Copy link
Member

Choose a reason for hiding this comment

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

how is it possible that these values would be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it more about ts not being able to recognize that we are exporting routes from each of these routes so when we want to use it it throws:

const routes: (navigation.types.Route<{}, {}> | navigation.types.Route<content.types.Params, State> | navigation.types.Route<Params, {}> | undefined)[]
Argument of type '(Route<{}, {}> | Route<Params, State> | Route<Params, {}> | undefined)[]' is not assignable to parameter of type 'AnyRoute[]'.
  Type 'Route<{}, {}> | Route<Params, State> | Route<Params, {}> | undefined' is not assignable to type 'AnyRoute'.
    Type 'undefined' is not assignable to type 'AnyRoute'.ts(2345)

I've used assertNonNullableArray name because I've also used NonNullable for export type AnyRoute = NonNullable<typeof routes[number]>;

Copy link
Member

Choose a reason for hiding this comment

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

what is actually causing that undefined to be in the type? is it one of the modules in particular that has it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is because of : /* istanbul ignore next */ {} in

...(
    process.env.REACT_APP_ENV !== 'production'
      ? developer.routes
      : /* istanbul ignore next */ {}
  ),

if I change it to:

...({}),

or to:

...(developer.routes),

then it works, so I guess this is a typescript problem

Copy link
Member

Choose a reason for hiding this comment

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

is it because of the ternary? that is very odd

it would be safe to cast the ternary result to typeof developer.routes if that helps


const init = [
...Object.values(auth.init),
Expand Down
7 changes: 7 additions & 0 deletions src/app/navigation/middleware.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ describe('navigation middleware', () => {

beforeEach(() => {
middleware = require('./middleware').default;

Object.defineProperty(assertWindow(), 'location', {
value: {
assign: jest.fn(),
replace: jest.fn(),
},
});
});

it('calls history when callHistoryMethod is dispatched', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/app/navigation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export interface LocationChange<M = AnyMatch> {
action: Action;
}

export type AnyRoute = typeof routes[number];
export type AnyRoute = NonNullable<typeof routes[number]>;
Copy link
Member

Choose a reason for hiding this comment

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

this isn't necessary anymore right?

export type AnyMatch = UnionRouteMatches<AnyRoute>;

export type RouteHookBody<R extends AnyRoute> = (helpers: MiddlewareAPI & AppServices) =>
Expand Down
9 changes: 7 additions & 2 deletions src/app/notifications/components/UpdatesAvailable.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,20 @@ describe('UpdatesAvailable', () => {
let reload: jest.SpyInstance;

beforeEach(() => {
reload = window!.location.reload = jest.fn();
reload = jest.fn();
Object.defineProperty(window, 'location', {
value: { reload },
});
resetModules();
({React, renderer, MessageProvider} = reactAndFriends());
UpdatesAvailable = require('./UpdatesAvailable').default; // tslint:disable-line:variable-name
serviceWorkerNeedsUpdate = jest.spyOn(require('../../../helpers/applicationUpdates'), 'serviceWorkerNeedsUpdate');
});

afterEach(() => {
window!.location.reload = reloadBackup;
Object.defineProperty(window, 'location', {
value: { reload: reloadBackup },
});
});

it('reloads on click', () => {
Expand Down
Loading