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

[Calendar] Remove promise based loading in favor of loading prop #1829

Merged
merged 5 commits into from
May 28, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,5 @@ coverage
# editors
.vs
.DS_Store

.vercel
1 change: 1 addition & 0 deletions docs/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.vercel
31 changes: 31 additions & 0 deletions docs/fakeApi/randomDate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { getDaysInMonth, isValid } from 'date-fns';
import { NowRequest, NowResponse } from '@now/node';

function getRandomNumber(min: number, max: number) {
return Math.round(Math.random() * (max - min) + min);
}

export default function(req: NowRequest, res: NowResponse) {
const { month } = req.query;
if (!month || typeof month !== 'string') {
res.status(400);
return res.json({
reason: 'month query param is required',
});
}

const date = new Date(month);
if (!isValid(date)) {
res.status(422);
return res.json({
reason: 'cannot parsable month value',
});
}

setTimeout(() => {
const daysInMonth = getDaysInMonth(date);
const daysToHighlight = [1, 2, 3].map(_ => getRandomNumber(1, daysInMonth));

res.json({ daysToHighlight });
}, 500); // fake some long work
}
2 changes: 2 additions & 0 deletions docs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"@material-ui/core": "^4.9.14",
"@material-ui/icons": "^4.9.1",
"@material-ui/pickers": "^4.0.0-alpha.1",
"@now/node": "^1.6.1",
"@types/fuzzy-search": "^2.1.0",
"@types/isomorphic-fetch": "^0.0.35",
"@types/jss": "^10.0.0",
Expand Down Expand Up @@ -57,6 +58,7 @@
"next-images": "^1.4.0",
"next-transpile-modules": "^2.0.0",
"notistack": "^0.9.11",
"now": "^19.0.1",
"prismjs": "^1.20.0",
"raw-loader": "^1.0.0",
"react": "^16.13.0",
Expand Down
30 changes: 18 additions & 12 deletions docs/pages/demo/datepicker/ServerRequest.example.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,29 @@ import { TextField } from '@material-ui/core';
import { DatePicker, Day } from '@material-ui/pickers';
import { makeJSDateObject } from '../../../utils/helpers';

function getRandomNumber(min, max) {
return Math.round(Math.random() * (max - min) + min);
}

function ServerRequest() {
const savedRequestRef = React.useRef(null);
const requestAbortController = React.useRef(null);
const [selectedDays, setSelectedDays] = React.useState([1, 2, 15]);
const [selectedDate, handleDateChange] = React.useState(new Date());

const handleMonthChange = () => {
window.clearTimeout(savedRequestRef.current)
setSelectedDays(null);
const handleMonthChange = date => {
if (requestAbortController.current) {
requestAbortController.current.abort();
}

setSelectedDays(null)

const controller = new AbortController();
fetch(`/fakeApi/randomDate?month=${date.toString()}`, {
signal: controller.signal,
})
.then(res => res.json())
.then(({ daysToHighlight }) => {
setSelectedDays(daysToHighlight);
Copy link
Member

Choose a reason for hiding this comment

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

What if the component unmounts before the request resolves? I believe the set state call will warn/throw. The solution proposed in could be applied here too mui/mui-x#5 (comment)

Copy link
Member Author

@dmtrKovalenko dmtrKovalenko May 25, 2020

Choose a reason for hiding this comment

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

We are saving abort controller. So needs to make an effect that will abort request on unmount. thanks

Copy link
Member

Choose a reason for hiding this comment

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

It's one way to solve this. I have no idea what's the best approach. I guess it's good enough, no need to look further 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, but I think that this solution has one giant pitfall – it doesn't abort the request.

Probably if we want to support IE we must not use fetch and promises at all.

Copy link
Member

Choose a reason for hiding this comment

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

Can requests be aborted? I mean, does it change something at the network layer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually request/response model cannot be aborted, but browsers can prevent some calculations on the responses if they are aborted (like processing low-level body and CORS)

Here network log when fast switching between months with request aborts:
image

Without:
image

Copy link
Member

Choose a reason for hiding this comment

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

Nice :)

})
.catch(() => console.log("Ooopsy, something went wrong"))

// just select random days to simulate server side based data
savedRequestRef.current = setTimeout(() => {
setSelectedDays([1, 2, 3].map(() => getRandomNumber(1, 28)));
}, 1000);
requestAbortController.current = controller;
};

return (
Expand Down
4 changes: 2 additions & 2 deletions lib/src/views/Calendar/CalendarView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import { DatePickerView } from '../../DatePicker';
import { useCalendarState } from './useCalendarState';
import { makeStyles } from '@material-ui/core/styles';
import { useUtils } from '../../_shared/hooks/useUtils';
import { VIEW_HEIGHT } from '../../constants/dimensions';
import { MaterialUiPickersDate } from '../../typings/date';
import { FadeTransitionGroup } from './FadeTransitionGroup';
import { Calendar, ExportedCalendarProps } from './Calendar';
import { PickerOnChangeFn } from '../../_shared/hooks/useViews';
import { withDefaultProps } from '../../_shared/withDefaultProps';
import { DAY_SIZE, DAY_MARGIN } from '../../constants/dimensions';
import { CalendarHeader, CalendarHeaderProps } from './CalendarHeader';
import { YearSelection, ExportedYearSelectionProps } from './YearSelection';
import { defaultMinDate, defaultMaxDate } from '../../constants/prop-types';
Expand Down Expand Up @@ -76,7 +76,7 @@ export const useStyles = makeStyles(
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
minHeight: VIEW_HEIGHT - 60,
minHeight: (DAY_SIZE + DAY_MARGIN * 4) * 7,
height: '100%',
},
},
Expand Down
12 changes: 11 additions & 1 deletion now.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,18 @@
"IS_NOW": "true"
}
},
"builds": [{ "src": "docs/next.config.js", "use": "@now/next" }],
"builds": [
{ "src": "docs/next.config.js", "use": "@now/next" },
{
"src": "docs/fakeApi/*",
"use": "@now/node"
}
],
"routes": [
{
"src": "/fakeApi/(.*)$",
"dest": "/docs/fakeApi/$1.ts"
},
{ "src": "/api/(?<name>[^/]+)$", "dest": "docs/api/props?component=$name" },
{ "src": "/(.*)", "dest": "docs/$1" }
]
Expand Down
15 changes: 11 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2468,6 +2468,13 @@
"@nodelib/fs.scandir" "2.1.2"
fastq "^1.6.0"

"@now/node@^1.6.1":
version "1.6.1"
resolved "https://registry.yarnpkg.com/@now/node/-/node-1.6.1.tgz#d478bfbc98af05d3eae7b5b01b26d1a1c638819b"
integrity sha512-EdSdOS4HSJRexibxIbrKCLZZ1sc0+oaD0P7kvhf26CbXSNlrZP0Iycpb+fbO4Qc2EcmGR2am90DhoX548B5a7g==
dependencies:
"@types/node" "*"

"@oclif/color@^0.0.0":
version "0.0.0"
resolved "https://registry.yarnpkg.com/@oclif/color/-/color-0.0.0.tgz#54939bbd16d1387511bf1a48ccda1a417248e6a9"
Expand Down Expand Up @@ -10157,10 +10164,10 @@ notistack@^0.9.11:
hoist-non-react-statics "^3.3.0"
react-is "^16.8.6"

now@^18.0.0:
version "18.0.0"
resolved "https://registry.yarnpkg.com/now/-/now-18.0.0.tgz#e6689205346e354f822b1f5feb2c0d7d570c172f"
integrity sha512-MVskFV3xH1hMkpTewPCb3p0SQ17hL7EI1Zb+Ij2wYiRV3X0a6G91GdE2vvFlhsK6rhRHKHZnDQkZ0AnkpnfzHA==
now@^19.0.1:
version "19.0.1"
resolved "https://registry.yarnpkg.com/now/-/now-19.0.1.tgz#a4575adfed17ea049d9207c145f3ce7f60c818cf"
integrity sha512-Q/dUlRBPzoy6lHw9P9qnzGmXw3aBU5ypQ1JjrKwoQJoEVhU5hCa9dJCEtIiSIG5cModcirU0xJPemg54dvWPnA==

npm-bundled@^1.0.1:
version "1.0.6"
Expand Down