rewrite shisell in typescript #36
Conversation
nataly87s
commented
Oct 4, 2018
•
nataly87s
commented
- rewrote the package in typescript
- added missing tests (currently 100% coverage)
- set up multiple build targets (cjs, es5, es2015, umd)
nataly87s
force-pushed
the
typescript
branch
3 times, most recently
from
021334d to
92d41d9
Compare
There was a problem hiding this comment.
Looks good generally, mostly nitpicking stuff.
However, I couldn't get any of the subpackages (filters, extenders, writers) to work in node, I think there need to be writers.js etc files in the published package for that to work.
Also - both src and tools folders are included in the package, is this on purpose? If so, the .d.ts files are not really needed - we can just direct typings to the ts source code.
.npmignore
Outdated
| @@ -1,8 +1,12 @@ | |||
| .idea | |||
There was a problem hiding this comment.
I know this was preexisting, but with al the stuff we're adding here - what do you think of using package.json's files as a whitelist instead of this blacklist?
src/internal/AnalyticsDispatcher.ts
Outdated
| readonly context: AnalyticsContext = new AnalyticsContext(), | ||
| ) {} | ||
|
|
||
| dispatch = (eventName: string = '', analyticsContext?: AnalyticsContext): T => { |
There was a problem hiding this comment.
Are dispatch and extend arrow functions instead of methods on purpose? Dispatchers are created a lot of times, this will cause each of them to create a new function
src/internal/AnalyticsContext.ts
Outdated
| return this; | ||
| } | ||
|
|
||
| const union = new AnalyticsContext(); |
There was a problem hiding this comment.
This bit of code is clever, but hard to read and throws type safety out the window.
Feels to me like this was far easier to understand and just as good: https://github.com/Soluto/shisell-js/blob/master/lib/AnalyticsDispatcher.js#L13
src/internal/extenders/utils.ts
Outdated
| return !!obj && typeof obj === 'object' && !Array.isArray(obj); | ||
| } | ||
|
|
||
| export function isArray(arr: any) { |
There was a problem hiding this comment.
Why not just Array.isArray?
src/internal/extenders/withExtras.ts
Outdated
| import {withContext} from './withContext'; | ||
|
|
||
| export function withExtras(extras: DataMap) { | ||
| if (!isObject(extras)) { |
There was a problem hiding this comment.
This seems like an error for sure, it's worth writing a log about it (in development mode)
Same for withIdentities/withFilters
There was a problem hiding this comment.
that's the previous behavior. if it were up to me I would throw an exception here
src/internal/extenders/utils.ts
Outdated
| return x; | ||
| } | ||
|
|
||
| export function isObject(obj: any) { |
There was a problem hiding this comment.
User-defined type guards were made to be used in this file 😄
Also, unknown fits better here then any.
to publish run `./build.sh && npm publish dist`
build.sh
Outdated
|
|
||
| rm -rf dist | ||
|
|
||
| yarn tsc -d |
There was a problem hiding this comment.
Why not add tslib dependency and use importHelpers? This way we don't need to emit async await etc helpers in the library.
