-
Notifications
You must be signed in to change notification settings - Fork 236
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
CoreUtils - Tree-Shaking ennhancements #1469
Conversation
MSNev
commented
Feb 8, 2021
- extract and export each CoreUtils function as first class
- stop using CoreUtils class within core, using the new direct functions
@@ -35,7 +35,8 @@ export { | |||
INotificationManager, | |||
IProcessTelemetryContext, | |||
Tags, | |||
BaseCore | |||
BaseCore, | |||
CoreUtils |
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.
Wasn't directly exported but was indirectly available because of the internal usages, so to avoid breaking anyone how had taken a dependency explicitly exporting.
* Config file for API Extractor. For more info, please visit: https://api-extractor.com | ||
*/ | ||
{ | ||
"$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json", |
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.
Adding more Api Extractor generations -- VERY useful for spotting the missed exports
|
||
// Using manual spies as sinon spy is failing to correctly restore the function | ||
let orgMwcRandom = CoreUtils.mwcRandom32; | ||
CoreUtils.mwcRandom32 = function() { |
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.
Generally, I tried to not change any of the Tests so that they continue to use the CoreUtils versions (to help ensure no-one is broken). However, in this case because it mocks the values but the internal code no longer uses these references these tests had to change to continue passing.
The minor side effect is that it's actually testing more of the production level execution path, by following the same rules for IE detection etc.
let randCalled = 0; | ||
// Check that mwcRandom is bing called (relies on the mwc implementation from the default seed) | ||
mwcRandomSeed(1); | ||
Assert.notEqual(722346555, random32(), "Make sure that the mwcRandom was not being called - step 1"); |
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.
Now also using "magic" numbers as mwc is psuedo random, so using the same seed always gives the same result (or should). So the test is also confirming that it really is using mwc.
@@ -32,7 +32,7 @@ export class AppInsightsCore extends BaseCore implements IAppInsightsCore { | |||
if (telemetryItem === null) { | |||
_notifyInvalidEvent(telemetryItem); | |||
// throw error | |||
throw Error("Invalid telemetry item"); | |||
throwError("Invalid telemetry item"); |
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.
Simply minification help as "throw" is a reserved word and "Error" is a class, so neither could be minified
arrForEach, arrIndexOf, arrMap, arrReduce, attachEvent, dateNow, detachEvent, hasOwnProperty, | ||
isArray, isBoolean, isDate, isError, isFunction, isNullOrUndefined, isNumber, isObject, isString, isTypeof, | ||
isUndefined, objDefineAccessors, objKeys, strTrim, toISOString | ||
} from "./HelperFuncs"; |
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.
Splitting these out to their own file to "fix" the cyclic dependencies build issues
} | ||
} | ||
} | ||
export interface ICoreUtils { |
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.
Defining an interface for the old "exported" CoreUtils class, so that CoreUtils can become an exported constant that follows the previously exposed class values that just reference the extracted functions -- i.e. CoreUtils should not have any locally defined functions.
There is 1 current exception to this and its the CoreUtils._canUseCookies property -- because it's a property. This is used by the community so it need to stay as a working change -- even when I add my CookieManager changes on top of this.
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.
So the changes in this file include
- moving functions from CoreUtils.xxx => first class function (defined and exported directly)
- moving the functions from CoreUtils.xxx => first class function that are now in HelperFuncs.ts and RandomHelper.ts etc
- Defining ICoreUtils interface to match and document the previous CoreUtils
- export a constant CoreUtils: ICoreUtils implementation (start @ line 365)
- Same for EventHelper class
public static _canUseCookies: boolean; | ||
/** | ||
* Internal - Do not use directly. | ||
* @deprecated Direct usage of this property is not recommend |
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.
More details will be added around this when the CookieManager changes are merged (plan is before next release)
/** | ||
* Identifies whether the current environment appears to be IE | ||
*/ | ||
export function isIE() { |
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.
Moved from CoreUtils.ts
const strDetachEvent = "detachEvent"; | ||
const strRemoveEventListener = "removeEventListener"; | ||
|
||
export function objToString(obj: 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.
Moved from CoreUtils.ts -- to avoid cyclic dependencies -- we have had these build warnings for a while, but this fixes that.
@@ -172,7 +172,7 @@ function _getObjProto(target:any) { | |||
function _getOwner(target:any, name:string, checkPrototype:boolean): any { | |||
let owner = null; | |||
if (target) { | |||
if (CoreUtils.hasOwnProperty(target, name)) { | |||
if (hasOwnProperty(target, name)) { |
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.
These changes are just a case of stopping direct usage of CoreUtils and use the functions directly
c54c314
to
26b6689
Compare
26b6689
to
034254c
Compare
- extract and export each CoreUtils function as first class - stop using CoreUtils class within core, using the new direct functions - create TestFramework and change core to use it (so it restores the navigator / user agent properly)
034254c
to
d4d27d7
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.
LGTM