Skip to content
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

Update to TypeScript 4.x #1071 #1130

Merged
merged 3 commits into from
Aug 16, 2021
Merged

Update to TypeScript 4.x #1071 #1130

merged 3 commits into from
Aug 16, 2021

Conversation

MSNev
Copy link
Collaborator

@MSNev MSNev commented Dec 4, 2019

Addresses #1071 and builds on Aaron's PR #1072

@MSNev MSNev force-pushed the MSNev/UpgradeTypeScript branch 2 times, most recently from 3e4d406 to a4bfd0d Compare December 6, 2019 22:49
@MSNev
Copy link
Collaborator Author

MSNev commented Dec 19, 2019

We had a discussion this morning about this change and we have decided to delay this until later in 2020 as the last time we attempt to upgrade to Typescript v3 we broke people using Typescript v2.

As such the current plan is that we will introduce a major upgrade next year (bumping the AI version to v3) along with some other work to assist with code minification that could additionall cause breaking changes (due to hiding of internal properties and potentially changing of methods from class (prototype) based to defined as instance level (like properties) myFunction: (theargs) => result; and supply the implementation in the constructor vs myFunction(theArgs): result { ... }

@hecflores
Copy link

Question. Will the current version (not using typescript 3.x) cause issues with users using typescript 3.x?

@MSNev
Copy link
Collaborator Author

MSNev commented Jan 21, 2020

@hecflores No, the current version using TS v2 will work fine for anyone using TS 3+.

My understanding from talking with the team is that the issue is related the generated *.d.ts files by TS 3.x and users using TS 2.x. The team had previously upgraded to TS 3,x (and rolled back) due to breaking issues that where reported.

@MSNev MSNev requested a review from a team as a code owner January 30, 2020 01:54
@MSNev MSNev force-pushed the MSNev/UpgradeTypeScript branch 4 times, most recently from f2833df to 266ad6e Compare June 9, 2020 19:21
@MSNev MSNev force-pushed the MSNev/UpgradeTypeScript branch 2 times, most recently from 60ba914 to 06329a8 Compare August 31, 2020 21:51
@MSNev MSNev force-pushed the MSNev/UpgradeTypeScript branch 2 times, most recently from fca52cb to 9a1d0f1 Compare November 21, 2020 01:44
@MSNev MSNev force-pushed the MSNev/UpgradeTypeScript branch 2 times, most recently from 9f4296f to 9e83e9b Compare April 28, 2021 20:17
@MSNev MSNev requested review from hectorhdzg, Karlie-777, kryalama, ramthi and xiao-lix and removed request for a team August 12, 2021 18:22
@MSNev MSNev added this to the 2.x.x (Next Release) milestone Aug 12, 2021
- run: npm install
- run: npm run build --if-present
- run: npm test
# - run: npm install
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of this change React and React-native extensions are now built and tested as part of the main ci.yml flow, so once this is checked in we will be able to build, release and test these all together.

- name: Run tests for ReactNative plugin
working-directory: extensions/applicationinsights-react-native
run: npm test
# - name: Install root dependencies
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, react-native is build and tested as part of the normal build again.

@@ -0,0 +1,361 @@
/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add api-extractor to react as it's now part of the main build.

@@ -5,7 +5,7 @@
"noImplicitAny": true,
"module": "es6",
"moduleResolution": "Node",
"target": "es3",
"target": "es5",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react doesn't work in IE, so this is a safe change (required for eslint validation)

import ReactPlugin from "./ReactPlugin";

export type AIReactCustomEvent<T> = Dispatch<SetStateAction<T>>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required for eslint validation

import dynamicProto from '@microsoft/dynamicproto-js';

export class ReactNativePluginTests extends TestClass {
export class ReactNativePluginTests extends AITestClass {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change reactnative tests to use the common test framework as it's now built and tested during the normal build.

const autoCollectStub = this.sandbox.stub(this.plugin, '_collectDeviceInfo');
const autoCollectExceptionStub = this.sandbox.stub(this.plugin, '_setExceptionHandler', () => true);
const autoCollectStub = this.sandbox.stub(this.plugin as any, '_collectDeviceInfo');
const autoCollectExceptionStub = this.sandbox.stub(this.plugin as any, '_setExceptionHandler').callsFake(() => true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change required as part of bumping to the latest version of Sinon which removed the previous stub function

@@ -175,21 +173,6 @@ export class ReactNativePluginTests extends TestClass {
private _getDevice(plugin: any): any {
return plugin._getDbgPlgTargets()[0];
}

protected _disableDynProtoBaseFuncs() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now in the common test framework class (AITestClass)

@@ -8,12 +8,12 @@
<link rel="stylesheet" href="https://code.jquery.com/qunit/qunit-1.23.1.css">
<script src="http://sinonjs.org/releases/sinon-2.3.8.js"></script>
<script src="http://cdnjs.cloudflare.com/ajax/libs/require.js/2.2.0/require.js"></script>
<script src="../../../../common/Tests/Selenium/ModuleLoader.js"></script>
<script src="../../../../common/Tests/Selenium/SimpleSyncPromise.js"></script>
<script src="../../../common/Tests/Selenium/ModuleLoader.js"></script>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required as re-organizing the test files to be the same as the main sku's and extensions

@@ -3,33 +3,43 @@
"version": "2.3.5",
"description": "Microsoft Application Insights React Native Plugin",
"main": "dist-esm/index.js",
"types": "dist-esm/index.d.ts",
"types": "types/index.d.ts",
Copy link
Collaborator Author

@MSNev MSNev Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing so all packages are dropping files in the same way.

"alwaysStrict": true,
"suppressImplicitAnyIndexErrors": true,
"allowSyntheticDefaultImports": true,
"importHelpers": true,
"noEmitHelpers": true,
"forceConsistentCasingInFileNames": true,
"declaration": true,
"declarationDir": "dist-esm",
"declarationDir": "extensions/applicationinsights-react-native/types",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to match the package.json location

@@ -66,6 +65,16 @@
"projectFolder": "extensions/applicationinsights-clickanalytics-js",
"shouldPublish": true
},
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add back react and react-native into the main build/test cycle

@MSNev MSNev merged commit f3f1a80 into master Aug 16, 2021
@MSNev MSNev deleted the MSNev/UpgradeTypeScript branch November 13, 2021 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants