-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add type definition #1116
Add type definition #1116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this built on my internal branch adding types to actions, or is this an independent effort? If not I might want to review that branch and see if it contains something additional.
package.json
Outdated
@@ -49,7 +49,8 @@ | |||
"docs": "babel-node ./scripts/documentation.js", | |||
"example-version": "babel-node ./scripts/edit-version.js", | |||
"prettier-all": "prettier --write '{src,examples,test}/**/*.js'", | |||
"lint": "eslint src test webpack examples/*.js examples/**/src website/*.js website/src --fix", | |||
"lint": "yarn typescript && eslint src test webpack examples/*.js examples/**/src website/*.js website/src --fix", | |||
"typescript": "tsc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear we can write tsc --noEmit
- though we do have the noEmit setting in the tsconfig, I still think it is worth having it here to emphasize that we are just type checking.
df878eb
to
f53431e
Compare
Signed-off-by: Shan He <heshan0131@gmail.com> add vis-state-updater Signed-off-by: Shan He <heshan0131@gmail.com> fix module resolve Signed-off-by: Shan He <heshan0131@gmail.com> wip type checking vis-state actions Signed-off-by: Shan He <heshan0131@gmail.com> filter-utiles type Signed-off-by: Shan He <heshan0131@gmail.com> vis-state-updater tpes Signed-off-by: Shan He <heshan0131@gmail.com> add merger schema Signed-off-by: Shan He <heshan0131@gmail.com>
de78e01
to
764a702
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Looking forward to this
src/layers/layer-factory.js
Outdated
@@ -28,14 +28,15 @@ export const PROPERTY_GROUPS = keyMirror({ | |||
stroke: null, | |||
radius: null, | |||
height: null, | |||
|
|||
angel: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: angle
src/layers/layer-factory.d.ts
Outdated
defaultValue: boolean; | ||
}; | ||
|
||
export type VisConfigSelection = VisConfig<string> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is VisConfig
a generic? If so, why is there VisConfig<string>
but not VisConfig<boolean>
? If not, shouldn't this line error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think VisConfig is a generic, this may be a typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a typo, good catch
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
/** | ||
* Input dataest parsed to addDataToMap | ||
*/ | ||
export type ProtoDataset = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why protodataset and not simply Dataset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the input data that user passed to addDAtaToMap, it then gets converted to dataset
* @param {string} payload.mapboxApiUrl - mapboxApiUrl to be saved to mapStyle reducer. | ||
* @param {Boolean} payload.mapStylesReplaceDefault - mapStylesReplaceDefault to be saved to mapStyle reducer | ||
* @param {object} payload | ||
* @param payload.mapboxApiAccessToken - mapboxApiAccessToken to be saved to mapStyle reducer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you remove types here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never mind i see the @type
src/actions/identity-actions.d.ts
Outdated
|
||
import ActionTypes from 'constants/action-types'; | ||
|
||
export function registerEntry(entry: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you create a a type for the entry inptu?
type RegistryEntity {
id:...
mint:...
...
}
src/actions/actions.d.ts
Outdated
|
||
export type AddDataToMaoPayload = { | ||
datasets: ProtoDataset[]; | ||
options?: AddDaataToMapOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type in AddDaataToMapOptions
|
||
export function addDataToMap( | ||
data: AddDataToMaoPayload | ||
): {type: ActionTypes.ADD_DATA_TO_MAP; payload: AddDataToMaoPayload}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo AddDataToMaoPayload
/** REQUEST_MAP_STYLES */ | ||
export type RequestMapStylesUpdaterAction = { | ||
payload: { | ||
[key: string]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use MapStyles?
export type MapStyles = {
[key: string]: BaseMapStyle;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the request payload, BaseMapStyle is the merged result of this request.
src/actions/provider-actions.d.ts
Outdated
provider: Provider; | ||
options?: ExportFileOptions; | ||
onSuccess?: any; | ||
closeModal?: booleam; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in booleam
src/actions/map-style-actions.d.ts
Outdated
/** LOAD_MAP_STYLES */ | ||
export type LoadMapStylesUpdaterAction = { | ||
payload: { | ||
[id: string]: BaseMapStyle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
src/actions/provider-actions.d.ts
Outdated
error: any; | ||
provider: Provider; | ||
options?: Options; | ||
onError?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(error: object) => void
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be better to create a type ErrorCallback since you are using it in few places
export type LoadCloudMapPayload = { | ||
loadParams: any; | ||
provider: string; | ||
onSuccess?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for this one
src/actions/provider-actions.d.ts
Outdated
loadParams: any; | ||
provider: string; | ||
onSuccess?: any; | ||
onError?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ErrorCallback
src/actions/provider-actions.d.ts
Outdated
response: any; | ||
loadParams: any; | ||
provider: string; | ||
onSuccess?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
src/actions/provider-actions.d.ts
Outdated
export type LoadCloudMapErrorPayload = { | ||
error: any; | ||
provider: stirng; | ||
onError?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
src/actions/vis-state-actions.d.ts
Outdated
|
||
export type LayerConfigChangeUpdaterAction = { | ||
oldLayer: Layer; | ||
newConfig: Partiel<LayerConfig>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a typo? should it be Partial? I am not sure
@@ -51,130 +50,120 @@ import {ACTION_PREFIX} from './default-settings'; | |||
* | |||
* export default createStore(reducers, {}, applyMiddleware(taskMiddleware)) | |||
*/ | |||
const ActionTypes = keyMirror({ | |||
const ActionTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be achieved by creating a helper that takes the map object and add the prefix to each and every property
Object.keys(types).reduce((acc, key) => ({
[key]: `${ACTION_PREFIX}${types[key]}`
}), {})
I mean you. already did the work so it is not a big deal but something to consider
src/reducers/vis-state-merger.d.ts
Outdated
export function mergeLayers(state: VisState, layers: ParsedConfig['visState']['layers']): VisState; | ||
export function mergeSplitMaps(state: VisState, splitMaps: ParsedConfig['visState']['splitMaps']): VisState; | ||
|
||
export function mergeInteractionTooltipConfig(state: VisState) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line is missing in this file
} from './schema-manager'; | ||
|
||
export const KeplerGlSchema: KeplerGLSchema; | ||
export default KeplerGlSchema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Work!
just few comments
Signed-off-by: Shan He <heshan0131@gmail.com>
Signed-off-by: Shan He <heshan0131@gmail.com>
Part 1: Added typedef to reducers, actions, processers, schemas, partially utils Signed-off-by: Shan He <heshan0131@gmail.com> Signed-off-by: Wesam Manassra <wesam.manassra@gmail.com>
Part 1 of the work: Added typedef to
Signed-off-by: Shan He heshan0131@gmail.com