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 8 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
69 changes: 0 additions & 69 deletions script/doctor-workbox.bash
Original file line number Diff line number Diff line change
Expand Up @@ -3,82 +3,13 @@
# 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

mkdir "$build"/books

mv "$worker" "$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
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
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
4 changes: 2 additions & 2 deletions src/app/developer/components/Routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { H3 } from '../../components/Typography';
import Panel from './Panel';

// tslint:disable-next-line:variable-name
const Routes: React.SFC = () => <Panel title='Routes'>
{routes.map((route) => <div key={route.name}>
const Routes = () => <Panel title='Routes'>
{routes.map((route) => route && <div key={route.name}>
<H3>{route.name}</H3>
path: {route.paths.map((path) => <React.Fragment key={path}>{path}<br /></React.Fragment>)}
</div>)}
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
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
8 changes: 4 additions & 4 deletions src/app/navigation/utils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ describe('matchPathname', () => {
});

it('renders a path with params', () => {
const spy = jest.spyOn(routes[1], 'getUrl');
const spy = jest.spyOn(routes[1]!, 'getUrl');
const params = {foo: 'bar'};

expect(matchPathname({route: routes[1], params} as unknown as AnyMatch)).toEqual('url2');
Expand All @@ -137,11 +137,11 @@ describe('matchPathname', () => {

describe('matchUrl', () => {
it('renders a url with just a path', () => {
expect(matchUrl({route: routes[0], params: {}, state: {}})).toEqual('url1');
expect(matchUrl({route: routes[0]!, params: {}, state: {}})).toEqual('url1');
});

it('renders url with path and query', () => {
expect(matchUrl({route: routes[2], params: {}, state: {}}))
expect(matchUrl({route: routes[2]!, params: {}, state: {}}))
.toEqual('url3?archive=https%3A%2F%2Farchive-content03.cnx.org');
});
});
Expand Down Expand Up @@ -229,7 +229,7 @@ describe('matchSearch', () => {
});

it('renders a url with search', () => {
const spy = jest.spyOn(routes[2], 'getSearch');
const spy = jest.spyOn(routes[2]!, 'getSearch');
const params = { foo: 'bar' } as unknown as Params;

expect(
Expand Down
6 changes: 3 additions & 3 deletions src/app/navigation/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ if (typeof(document) !== 'undefined') {
const delimiter = '_';

export const matchForRoute = <R extends AnyRoute>(route: R, match: AnyMatch | undefined): match is Match<R> =>
!!match && match.route.name === route.name;
!!match && !!route && match.route.name === route.name;

export const locationChangeForRoute = <R extends AnyRoute>(
route: R,
locationChange: LocationChange
): locationChange is Required<LocationChange<Match<R>>> =>
!!locationChange.match && locationChange.match.route.name === route.name;
!!locationChange.match && !!route && locationChange.match.route.name === route.name;

export const getUrlRegexParams = (obj: object): object => flatten(obj, {delimiter});

Expand All @@ -56,7 +56,7 @@ const formatRouteMatch = <R extends AnyRoute>(route: R, state: RouteState<R>, ke

export const findRouteMatch = (routes: AnyRoute[], location: Location): AnyMatch | undefined => {
for (const route of routes) {
for (const path of route.paths) {
for (const path of route!.paths) {
const keys: Key[] = [];
const re = pathToRegexp(path, keys, {end: true});
const match = re.exec(location.pathname);
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