feat: rename noReplay to replay; set to true by default #71
Conversation
src/lib/api/inline.preboot.code.ts
Outdated
| ] | ||
| ]).map(eventSelector => assign(eventSelector, { | ||
| // set `replay` to `true` if not specified | ||
| replay: eventSelector.replay != null ? eventSelector.replay : true |
There was a problem hiding this comment.
I hesitate about doing this here, since it's a lot of work for those adding their own selectors to always add replay. Can you add this logic to the event replayer instead?
Something like if(!!eventSelector.replay) instead of if(!eventSelector.noReplay)
There was a problem hiding this comment.
This won’t be enough as undefinedis falsy. I’d have to compare to undefined explicitly. I’m a little afraid it might be fragile.
There was a problem hiding this comment.
Ah, I misread your comment. You meant it’s work for consumers, not Prevoot code. I agree then.
src/lib/api/inline.preboot.code.ts
Outdated
| @@ -111,9 +115,7 @@ export function getEventRecorderCode(): string { | |||
| * @param customOptions PrebootRecordOptions that override the defaults | |||
| * @returns Generated inline preboot code is returned | |||
| */ | |||
| export function getInlinePrebootCode(customOptions?: PrebootOptions): string { | |||
| const opts = <PrebootOptions>assign({}, defaultOptions, customOptions); | |||
There was a problem hiding this comment.
I don't want to move this because the Module is not the only consumer for this (ie React users may call it manually, and still want the default options)
There was a problem hiding this comment.
I see. Should I extend in both places or add an undefined check?
My concern with checking for undefined is that it breaks if the default changes.
There was a problem hiding this comment.
When would the default change?
There was a problem hiding this comment.
Maybe it's not a big problem... Is extending in both places OK?
There was a problem hiding this comment.
Why would we need it in both places if the inlinePrebootCode handles it anyway?
src/lib/module.ts
Outdated
| nonce: string|null, | ||
| platformId: Object, | ||
| appRef: ApplicationRef, | ||
| eventReplayer: EventReplayer) { | ||
| // necessary because of angular/angular/issues/14485 | ||
| const res = () => { | ||
| // Use `Object.create(null)` as a base as otherwise user can modify | ||
| // the prototype of the options object. | ||
| const prebootOpts = <PrebootOptions>assign(Object.create(null), defaultOptions, customOpts); |
There was a problem hiding this comment.
This was a good idea, but in the future, since this is in the Angular module part of the app (and thus gets transpiled by the end user), you can just do an object spread here.
const prebootOpts = {...defaultOptions, ...customOpts};
There was a problem hiding this comment.
Actually, I can't do it as TypeScript's implementation of object spread is nonconformant and does regular assignments instead of Object.defineProperty and suffers from the same __proto__ prototype mutation issue as Object.assign with a regular object. :(
See here
There was a problem hiding this comment.
I think if someone goes to the trouble to typecast an object to PrebootOptions and replace the __proto__, maybe they deserve to cause errors?
CaerusKaru
force-pushed
the
no-replay
branch
from
ab146c6 to
06d43db
Compare
Fixes #69
noReplayso this sounds like a separate issue.An API change.
See #69
See #69
Yes, but Preboot 6 requires a different way of importing inside of an Angular NgModule so people upgrading will have to change their imports anyway.