Adds TS support #253
Adds TS support #253
Conversation
NejcZdovc
force-pushed
the
ts
branch
3 times, most recently
from
c55c091 to
a36c79a
Compare
Adds ts-loader; Removes standard
|
|
||
| export const NumBlockedStat = (props: Props) => ( | ||
| <div> | ||
| <span i18n-content='adsBlocked'/> {props.adsBlockedStat || 0} |
There was a problem hiding this comment.
no we can remove it
| // Utils | ||
| import * as storage from '../storage' | ||
|
|
||
| const adblockReducer: Reducer<AdBlock.State | undefined> = (state: AdBlock.State | undefined, action) => { |
There was a problem hiding this comment.
why are we allowing an undefined state here? maybe setting a default state or empty object would be better?
There was a problem hiding this comment.
Redux will send you undefined state in when you combine reducers https://github.com/reduxjs/redux/blob/9b43c56b98cc75505e161b8e26c18240fa99b184/src/combineReducers.js#L68. For every reducer we have check in the beginning for undefined. I am planing of improving this, so that we would will actually set state when redux send INIT in
| if (state === undefined) { | ||
| state = storage.load() || {} | ||
| state = Object.assign(storage.getInitialState(), state) | ||
| state = storage.load() |
There was a problem hiding this comment.
having a fallback to {} seemed a good guard against undefined values not sure why was removed
There was a problem hiding this comment.
Load can't send undefined so that's why it was removed. If we add guard then we need to allow empty object on state as well
| const starIcon = isBookmarked ? 'fa-star' : 'fa-star-o' | ||
| const pinIcon = isPinned ? 'fa-minus' : 'fa-thumb-tack' | ||
|
|
||
| return connectDragSource(connectDropTarget( | ||
| <div className='topSiteSquareSpace'> | ||
| <div | ||
| className='topSitesElement' | ||
| style={{ | ||
| opacity: opacity |
There was a problem hiding this comment.
was this removed on purpose?
| }, () => { | ||
| fetchBookmarkInfo(action.url) | ||
| }) | ||
| const topSite: NewTab.Site | undefined = state.topSites.find((site) => site.url === action.url) |
There was a problem hiding this comment.
I'm often seeing this pattern and wondering if we can find a better way to block undefined values via TS so it can warn us when something is wrong? Isn't this bypassing empty state values?
| if (state === undefined) { | ||
| state = storage.load() || {} | ||
| state = Object.assign(storage.getInitialState(), state) | ||
| state = storage.load() |
There was a problem hiding this comment.
same as above comment having a fallback to {} seemed a good guard against undefined values. please educate me if this is not need for some reason
| if (state === undefined) { | ||
| state = storage.load() || {} | ||
| state = Object.assign(storage.getInitialState(), state) | ||
| state = storage.load() |
There was a problem hiding this comment.
same as previous comment regarding fallback
| @@ -30,7 +26,7 @@ module.exports = { | |||
| rules: [ | |||
| { | |||
| test: /\.tsx?$/, | |||
| loader: 'awesome-typescript-loader' | |||
| loader: 'ts-loader' | |||
There was a problem hiding this comment.
is ts-loader just better than awesome-typescript-loader? wondering in case we should use it in brave-ui too
There was a problem hiding this comment.
I had quite some problems with awesome loader, because of brave-browser -> brave-core relation and files were not found. With ts-loader everything is working ok
| "tslint": "^5.8.0", | ||
| "tslint-config-standard": "^7.0.0", | ||
| "tslint-react": "^3.2.0", | ||
| "typescript": "^2.8.1" |
There was a problem hiding this comment.
we have these deps required in brave-browser as well, is the duplicate necessary? I'd like to have them here instead of the former as they're only for webUI and it's easier to review changes in one PR. I don't think they're needed in both places but cc @bbondy in case we need both
There was a problem hiding this comment.
I would like to have it live only in brave-core, because here an actual logic is and it' needed here because of travis, but the problem is that we are triggering builds from brave-browser repo. I think that we would need to change this logic and instead of triggering it from brave-browser webpack should be triggered directly from brave-core. Let me try it and see if I will be successful
| @@ -0,0 +1,28 @@ | |||
| { | |||
| "compilerOptions": { | |||
There was a problem hiding this comment.
same as above comment not sure if we need two tsconfig files this could lead to weird bugs in case we update only one and build is unsure which one to request
There was a problem hiding this comment.
comment left above
Ads tipsMigrationAlert component
Resolves brave/brave-browser#559
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist: