-
Notifications
You must be signed in to change notification settings - Fork 240
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
[BUG]AppInsights not working in IE7 #1142 #1162
Conversation
MSNev
commented
Jan 18, 2020
- Provide packaging rollup plugins to replace and detect non-ES3 included code.
@@ -31,7 +32,9 @@ const browserRollupConfigFactory = (isProduction, libVersion = '2') => { | |||
nodeResolve({ | |||
browser: false, | |||
preferBuiltins: false | |||
}) | |||
}), | |||
es3Poly(), |
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.
New rollup plugins that replace some of the incompatible ES3 code and check that we are not including some of the easily detectable known methods.
@@ -13,7 +13,7 @@ function _logWarn(aiName:string, message:string) { | |||
// TODO: Find better place to warn to console when SDK initialization fails | |||
var _console = typeof console !== Undefined ? console : null; | |||
if (_console && _console.warn) { | |||
_console.warn('Failed to initialize AppInsights JS SDK for instance ' + aiName + message); | |||
_console.warn('Failed to initialize AppInsights JS SDK for instance ' + (aiName || '<unknown>') + ' - ' + message); |
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.
Just formatting this error a little better, which mostly occurs on when the environment doesn't contain "window" or "JSON" implementations.
@@ -132,6 +132,16 @@ module.exports = function (grunt) { | |||
], | |||
out: './channels/applicationinsights-channel-js/Tests/Selenium/aichannel.tests.js' | |||
}, | |||
rollupes3: { |
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.
Add new plugins to the build chain
@@ -0,0 +1,73 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. |
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 es3Check() plugin
@@ -0,0 +1,141 @@ | |||
// Copyright (c) Microsoft Corporation. All rights reserved. |
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 es3Poly() plugin
// Licensed under the MIT License. | ||
import { IEs3CheckKeyword, IEs3Keyword } from "./Interfaces"; | ||
|
||
export const defaultEs3Tokens:Array<IEs3Keyword> = [ |
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.
The default keyword rules for es3Poly()
} | ||
]; | ||
|
||
export const defaultEs3CheckTokens:Array<IEs3CheckKeyword> = [ |
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.
The default keyword rules for es3Check()
funcNames: [ /Object\.(defineProperties)\(/g ], | ||
errorMsg: "[%funcName%] is not supported in an ES3 environment, use a helper function or add explicit check for existence", | ||
ignoreIds: [ | ||
"applicationinsights-react-js", // Don't break build if these exist in the final react extension |
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.
Ignoring react as the react code and included dependencies uses defineProperties
import { padEnd, isNullOrWhitespace } from "./Utils"; | ||
import { IEs3CheckKeyword, IEs3Keyword } from "./Interfaces"; | ||
|
||
export function formatError(keyword:IEs3CheckKeyword|IEs3Keyword, funcName:string, errorMsg:string, code:string, pos:number, id:string, entry: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.
Error formatter, it seems a bit over the top, but when it detects a failure this provides the context to help find the offending implementation.
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.
Self reviewed
2fbfcd5
to
2786af3
Compare
AISKU/package.json
Outdated
"@microsoft/applicationinsights-properties-js": "2.4.0-beta" | ||
}, | ||
"peerDependencies": { | ||
"@microsoft/applicationinsights-rollup-es3" : "1.0.0", | ||
"tslib": "^1.9.3" |
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.
tslib should be a pure dependency
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 as suggested
AISKU/package.json
Outdated
"@microsoft/applicationinsights-properties-js": "2.4.0-beta" | ||
}, | ||
"peerDependencies": { | ||
"@microsoft/applicationinsights-rollup-es3" : "1.0.0", |
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.
Does this need to be a peer dependency? Would it not work as a dev dependency?
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 as suggested
AISKULight/package.json
Outdated
"@microsoft/applicationinsights-common": "2.4.0-beta", | ||
"@microsoft/applicationinsights-channel-js": "2.4.0-beta", | ||
"@microsoft/applicationinsights-core-js": "2.4.0-beta" | ||
}, | ||
"peerDependencies": { | ||
"tslib": "^1.9.3" | ||
"@microsoft/applicationinsights-rollup-es3" : "1.0.0" |
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.
Does this need to be a peer dependency?
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 as suggested
"@microsoft/applicationinsights-common": "2.4.0-beta" | ||
}, | ||
"peerDependencies": { | ||
"@microsoft/applicationinsights-rollup-es3" : "1.0.0", |
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.
See other comments. I think this can be a dev dep
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 as suggested
"@microsoft/applicationinsights-common": "2.4.0-beta" | ||
}, | ||
"peerDependencies": { | ||
"@microsoft/applicationinsights-rollup-es3" : "1.0.0", | ||
"tslib": "^1.9.3" |
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.
see other tslib comments
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 as suggested
tools/rollup-es3/Tests/tsconfig.json
Outdated
"target": "es5", | ||
"alwaysStrict": true, | ||
"declaration": true, | ||
"lib": [ "dom", "es2015" ] |
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.
are these required?
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.
Yes, the test project needs it's own tsconfig
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 dom and es2015 required? This does not exist on other tsconfig in this repo
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.
Removed, but not sure while still listed here...
d21dcb2
to
a31cd9f
Compare
- Provide packaging rollup plugins to replace and detect non-ES3 included code. - Remove Es3 checks from reactive-native
a31cd9f
to
324a6d7
Compare