Merge React 18 feature branch #3050

JamesBurnside · 2023-05-15T18:11:36Z

What

Merge of base PRs:

Why

Merge short term solution for react 18, see #2939 & #1900.

How Tested

Selfhosting and bug bashes.

…t-term-support

github-actions · 2023-05-15T18:18:44Z

Storybook URL https://60c7ae6891f0e90039d7cd54-gvdqmycllc.chromatic.com/

github-actions · 2023-05-15T18:22:30Z

Calling bundle size is decreased✅.

Current size: 9813746
Base size: 9815637
Diff size: -1891

github-actions · 2023-05-15T18:22:36Z

CallWithChat bundle size is decreased✅.

Current size: 10227599
Base size: 10228351
Diff size: -752

github-actions · 2023-05-15T18:22:36Z

Chat bundle size is increased❗.

Current size: 9873045
Base size: 9872844
Diff size: 201

…t-term-support

github-actions · 2023-05-19T22:16:22Z

Storybook URL https://60c7ae6891f0e90039d7cd54-rbhthnvvaz.chromatic.com/

…t-term-support

github-actions · 2023-05-23T19:39:40Z

Storybook URL https://60c7ae6891f0e90039d7cd54-gdyjkudwlr.chromatic.com/

github-actions · 2023-05-23T20:00:27Z

Storybook URL https://60c7ae6891f0e90039d7cd54-pbxgobftpc.chromatic.com/

github-actions · 2023-05-23T20:42:14Z

Storybook URL https://60c7ae6891f0e90039d7cd54-opadlncfvz.chromatic.com/

github-actions · 2023-05-23T23:20:31Z

Storybook URL https://60c7ae6891f0e90039d7cd54-bhbbrcgufu.chromatic.com/

PorterNan · 2023-05-24T07:42:50Z

packages/acs-ui-common/package.json

  },
  "devDependencies": {
    "@babel/cli": "~7.16.0",
    "@babel/core": "~7.16.0",
    "@microsoft/api-documenter": "~7.12.11",
    "@microsoft/api-extractor": "~7.18.0",
    "@types/jest": "~26.0.22",
-    "@types/react": "^16.9.49",
+    "@types/react": "18.2.0",


Do we need to run react 16 compatibility test by any chance?
BTW, why not use ^ to get latest update for react 18

This should be pinned to the same as react so this should be whatever react is. Really they should probably all be ~ or ^ but we don't have good repo-wide guidenence on what developers should choose. Should we set a standard now and update guidence in the wiki?

And yeah we will need that, I have something cooking up on the side but haven't had time for it, will be similar to the typescript check

Yea, would be nice to always get latest bit of the current compatible version, cause it might be something unexpected happening there (very rare though) then we can catch it earlier than our customer (cause customers prefer those "^" for versions)

I will keep this change for a separate PR, made a bug for it for tracking Bug 3279379: [Web] Repo should be consistent and have guidence on using a pinned dep version vs ^ or ~

PorterNan · 2023-05-24T07:44:07Z

packages/communication-react/package.json

    "build:watch": "",
+    "copy-northstar": "cpx \"../northstar-wrapper/dist/dist-esm/**/*\" dist/dist-esm/northstar-wrapper/src --clean",


Just out of curiosity, why do we need to do this copy and paste?

We need the webpack output from the build. Currently our meta package rebuilds everything itself and hence tsc doesn't use the webpack output. This was the simplest way I found to have the webpack bits in the final npm package (but very open if you find a way to instead have tsc grab the webpack output instead of retranspiling the src code, pointing tsconfig straight at the dist output wasn't working :/)

Yea.. within the internal packages, it will try to use src folder instead (and that's why we use copy-preprocess), btw, I guess doing copy-northstar in the end means we want it to be packaged and consumed by customer, but our own build process doesn't need it?

Yeah that's correct, it does mean we have gaps where parts of our build system is using the webpack output and other parts just directly grabs the n* code from the northstar-wrapper/node_modules :/

PorterNan · 2023-05-24T07:48:07Z

packages/northstar-wrapper/src/index.ts

@@ -1,8 +1,19 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.

-import * as northStar from '@fluentui/react-northstar';
+import {


Any concerns that we don't do import * ? is it for treeshaking

The question is why we would do * at all. We don't want to allow exports from n* so we want to keep this to a minimal set such that we don't allow developers to accidentally introduce new reliances on northstar

So this is just what we are wrapping up? if someone were to add to this list would anything else need to be done or would it just auto pull it at build time?

Nothing needs done it will automatically work - but actually wish it didn't just work. Someone should not be adding to this list. We want to get further away from n* not be more reliant on it

Are we saying we developers could import NorthStar component from our package? I guess we don't expose them as a public api right?

nope, just internal developers to this repo, not npm package users

PorterNan · 2023-05-24T07:51:25Z

packages/northstar-wrapper/package.json

@@ -3,36 +3,43 @@
  "version": "1.5.1-beta.4",
  "private": "true",
  "description": "Package to wrap the Fluent NorthStar UI library, for short-term React 18 compatibility",
+  "module": "dist/dist-esm/index.js",


I guess we will generate both esm and cjs code in our npm package? How much bigger our npm packages will be in the end

Yeah, cjs is needed for anything that runs in node as it is still lagging behind on the esm support. Really, production builds of sites should always be using the esm output as it supports tree shaking and cjs should only be used for node if at all.
Re: size of the npm package, do we care? the package will be slightly bigger, but the npm dependencies will be far smaller as it no longer needs to pull in all of n* and all its dependencies

Yea... I agree with production build should always use esm, but still there would be some app living in cjs world for some legacy reasons, so we might still want to support that well

For npm package, that is just a question out of curiosity, I did receive a feedback from webchat team asking about the npm package size, but it shouldn't matter too much in the end

True, and we do support cjs, we just don't optimize any bundles. However anyone using this in cjs will actually receive a benefit because they don't have to rollup the entire n* library, the small pieces needed are now directly bundled by us so smaller package overall (unless of course they use both us and n* in their app and are bundling using cjs, then some n* parts will be bundled twice)

PorterNan · 2023-05-24T07:51:55Z

packages/northstar-wrapper/package.json

  "scripts": {
-    "build": "tsc",
+    "build": "rushx build:esm && rushx build:cjs",
+    "build:esm": "tsc && webpack -c ./webpack.esm.config.js && rushx api-extractor",


If that is esm, will webpack bundle them into one file or several files like all the other packages?

Still one file, but a file that has treeshaking support (treeshaking can still strip out unused exports in the same file)

Lets open a work item to validate that later including find a solution for treeshaking localization part

emlynmac

Just some questions, but looks good!

emlynmac · 2023-05-24T19:06:53Z

packages/react-composites/tests/browser/call/hermetic/VerticalGalleryResizing.test.ts

@@ -87,7 +87,7 @@ test.describe('Vertical gallery resizing tests', async () => {
    await waitForSelector(page, dataUiId(IDS.verticalGalleryVideoTile));

    // check initial tile number to be correct.
-    expect(await page.locator(dataUiId(IDS.verticalGalleryVideoTile)).count()).toBe(2);
+    await waitForSelectorCount(page, dataUiId(IDS.verticalGalleryVideoTile), 2);


So do we no longer need to expect things?

in this case nope, it's updated to instead wait for the correct count and fails if the condition isn't met. Change here allows two things:

the count is now waited for (so can be in subsequent render passes)

the action will now screenshot on a failure

So previous one will be waiting for dataUiId(IDS.verticalGalleryVideoTile), if that is showing up, it will start counting the number

But the current behavior would be wait until it is 2 count, and failed it that is not (and take screenshot I guess?)

I am good with this one, this might miss some "delay render"(like first second it renders one element, but later render 2 elements which meet the condition) issues, but also allow use to get more stable UI snapshot

It could have either make our pipeline faster or slower, would like to see the final number when we have this in main

I believe the previous one actually did no waiting, it just immediately checked what was rendered. We now instead wait for the condition to become true because the resize observer kicks in after mounting so the tiles we show changes across a couple render passes

...ipantPane.test.ts-snapshots/PSTN-call-screen-with-dialpad-Mobile-Android-Landscape-linux.png

emlynmac · 2023-05-24T19:08:45Z

packages/storybook/stories/storybook.test.ts

@@ -45,7 +45,7 @@ afterAll(() => {
  console.error = consoleError;
 });

-describe('storybook snapshot tests', () => {
+describe.skip('storybook snapshot tests', () => {


Maybe storybook 7 works?

github-actions · 2023-05-24T21:13:02Z

Storybook URL https://60c7ae6891f0e90039d7cd54-kyqjorsqos.chromatic.com/

dmceachernmsft · 2023-05-24T21:37:09Z

packages/northstar-wrapper/src/index.ts

@@ -1,8 +1,19 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.

-import * as northStar from '@fluentui/react-northstar';
+import {


So this is just what we are wrapping up? if someone were to add to this list would anything else need to be done or would it just auto pull it at build time?

dmceachernmsft · 2023-05-24T21:58:38Z

packages/react-components/src/components/Announcer/AnnouncerContext.ts

+};
+
+/** @private */
+const AnnouncerContext = React.createContext<AnnouncerContextType>({


Just thinking about your LIME investigation (still sad no margaritas) is there any concern about more contexts? even though this is more a N* thing?

I will leave this to a later discussion. This doesn't add a new provider, just moves it from an external package into our codebase

dmceachernmsft · 2023-05-24T22:09:52Z

packages/react-components/src/components/MessageStatusIndicator.tsx

@@ -123,7 +123,7 @@ export const MessageStatusIndicator = (props: MessageStatusIndicatorProps): JSX.
          calloutProps={{ ...calloutProps }}
          styles={hostStyles}
        >
-          {strings.sendingAriaLabel && <LiveMessage message={strings.sendingAriaLabel} aria-live="polite" />}
+          {strings.sendingAriaLabel && <LiveMessage message={strings.sendingAriaLabel} ariaLive="polite" />}


Small thing but did we do any announcer testing here? we have had bugs around this component in the past not announcing states

Just the testing done here: #3005. Confirmed the DOM shows the same behavior as before

PorterNan · 2023-05-24T22:15:32Z

packages/communication-react/tsconfig.preprocess.json

@@ -10,7 +10,8 @@
      "@internal/calling-stateful-client": ["calling-stateful-client/src"],
      "@internal/chat-stateful-client": ["chat-stateful-client/src"],
      "@internal/react-components": ["react-components/src"],
-      "@internal/react-composites": ["react-composites/src"]
+      "@internal/react-composites": ["react-composites/src"],
+      "@internal/northstar-wrapper": ["northstar-wrapper/src"]


This is for preprocess version, do we preprocess northstar as well?

There's no conditional compiles for n* wrapper package, and given we should not be introducing new usage of this library there shouldn't be in future so stable vs beta didn't need supported in this package

(so I guess this line isn't needed? I just copy-pasted anywhere @internal packages were referenced)

PorterNan · 2023-05-24T22:24:41Z

packages/react-components/src/components/Announcer/Announcer.tsx

@@ -0,0 +1,66 @@
+// Copyright (c) Microsoft Corporation.


The reason why we have these files is because it's not exportable from northstar, I guess?

See here: #3005. The component we used has react 16 as peer dependency but it is a tiny package so we are just taking the code from it directly

PorterNan · 2023-05-24T22:26:12Z

packages/react-components/src/components/VideoGallery/utils/videoGalleryLayoutUtils.test.tsx

-    act(() => {
-      root.update(<TestComponent {...updatedArgs} />);
-    });
+    rerender(<TestComponent {...updatedArgs} />);


This is the new format from new react test package?

Sry the render one

Ohh, you mentioned react 18 change its way to render components

Yeah newer syntax with the newer testing library. the render blocks don't need to be in act blocks

PorterNan · 2023-05-24T22:28:56Z

packages/react-composites/tests/app/chat/index.tsx

@@ -16,4 +16,9 @@ const params = Object.fromEntries(urlSearchParams.entries());
 initializeFileTypeIcons();
 initializeIconsForUITests();

-ReactDOM.render(params.fakeChatAdapterArgs ? <FakeAdapterApp /> : <LiveTestApp />, document.getElementById('root'));
+const domNode = document.getElementById('root');


Yay, with this code piece, I guess we are good to use newest CRA to create projects!

dmceachernmsft · 2023-05-24T22:16:50Z

packages/react-composites/src/composites/CallComposite/MockCallAdapter.ts

@@ -98,7 +98,7 @@ export class MockCallAdapter implements CallAdapter {
    throw Error('createStreamView not implemented');
  }
  disposeStreamView(): Promise<void> {
-    throw Error('disposeStreamView not implemented');
+    return Promise.resolve();


why do we need an implementation here now? is there one needed for createView as well?

One of the tests called it as the tile logic now completes its unmount before the test finishes

ahhh okay that makes sense

dmceachernmsft · 2023-05-24T22:20:50Z

packages/react-composites/tests/app/lib/HiddenChatComposites.tsx

@@ -12,23 +12,67 @@ import { ChatAdapter, ChatComposite } from '../../../src';
 * The added chat composites are hidden, but tests can interact with them programmatically.
 */
 export const HiddenChatComposites = (props: { adapters: ChatAdapter[] }): JSX.Element => {
+  const [hiddenCompositesProps, setHiddenCompositesProps] = useState<HiddenChatCompositesProps[]>([]);


maybe dumb question but what is the reason for the hidden composites again? just to send messages in the tests? (wont block this PR on this but would love to know why we have these)

Yeah just to send messages and read receipts. They don't really have to be hidden tbh, and I wonder if we could replace them with just adapter uses and fire off read events and messages and such through the adapter interface

yeah could be a interesting refactoring of the tests. I don't think its anything worth blocking this over though since are already using this.

dmceachernmsft · 2023-05-24T22:21:28Z

packages/react-composites/tests/browser/call/hermetic/HorizontalGalleryResizing.test.ts

@@ -87,13 +87,13 @@ test.describe('Height gallery resizing tests', async () => {
    await waitForSelector(page, dataUiId(IDS.horizontalGalleryVideoTile));

    // check initial tile number to be correct.
-    expect(await page.locator(dataUiId(IDS.horizontalGalleryVideoTile)).count()).toBe(2);
+    await waitForSelectorCount(page, dataUiId(IDS.horizontalGalleryVideoTile), 2);


love this refactoring

dmceachernmsft · 2023-05-24T22:30:54Z

...ipantPane.test.ts-snapshots/PSTN-call-screen-with-dialpad-Mobile-Android-Landscape-linux.png

Did we always have this CSS when the box has focus? I thought the underline just turned blue

I believe you already asked this one 😉 #3045 (comment)

I did and totally missed it! that makes sense to me. I am also just remembering the CSS as it turns out!

PorterNan · 2023-05-24T22:39:33Z

packages/react-composites/tests/browser/callwithchat/hermetic/Chat.test.ts

    // Wait for indicator to show up
    await waitForAndHideTypingIndicator(page, APP_UNDER_TEST_ROOT_SELECTOR);
    // Stop typing and send a message
-    await stopTypingAndSendMessageFromHiddenChatComposite(page, chatRemoteParticipant);
+    await typeAndSendMessageFromHiddenChatComposite(page, chatRemoteParticipant, 'I agree!');


Looks like we are typing 'I agree' for twice and I guess this is changing the test behaivor?

The logic for how the hidden composite is hidden has changed. We now no longer render it entirely whereas before we just marked visibility as hidden. This is because react is much better now and hooks like triggering read receipts get triggered when the composite is hidden now. Because we unmount anything typed and not sent it lost, so I've updated the functions to by type* and typeAndSend*

Could be a work item not to use hidden composite but adapter/clients

PorterNan · 2023-05-24T22:40:49Z

packages/react-composites/tests/browser/common/hermeticChatTestHelpers.ts

-    return;
+    // wait for messages to have loaded
+    await waitForSelector(page, dataUiId('chat-composite-message'));
+    // sleep for 100ms to give time for the messagethread hook to send the read receipt


Maybe we need a hidden element in html to notify that "I have successfully sent the read receipt at what time point"

Yeah :/ Would be nice to use the adapter and not composites for these actually and use adapter interface to send the read receipt

PorterNan · 2023-05-24T22:41:26Z

...ipantPane.test.ts-snapshots/PSTN-call-screen-with-dialpad-Mobile-Android-Landscape-linux.png

Noticed there is a focus change here, is it by designed?

Yep see here: #3045 (comment)

dmceachernmsft · 2023-05-24T22:56:27Z

samples/Chat/tsconfig.json

@@ -9,6 +9,7 @@
      "@internal/acs-ui-common": ["../../../packages/acs-ui-common/src"],
      "@internal/calling-stateful-client": ["../../../packages/calling-stateful-client/src"],
      "@internal/chat-stateful-client": ["../../../packages/chat-stateful-client/src"],
+      "@internal/northstar-wrapper": ["../../../packages/northstar-wrapper/src"],


Maybe dumb question but do we want to be creating this linking for the samples? I thought we avoided the internal imports?

Good question! We avoid imports in the code to make sure we only use publicly available exports so that when we migrate the code to the samples nothing breaks. But internally it needs to reference the packlets for quicker building (doesn't have to first build the downstream packlets) and for hot reloading

github-actions · 2023-05-24T23:20:59Z

Storybook URL https://60c7ae6891f0e90039d7cd54-wfhhytqsqc.chromatic.com/

JamesBurnside added 5 commits May 1, 2023 14:46

react-aria-live react 18 support (#3005)

0300a63

Replace fluent n* with js bundle (#2988)

15c2622

Merge remote-tracking branch 'origin/main' into feature/react-18-shor…

8e97035

…t-term-support

Bump dependencies to react 18 (#3045)

30ecedc

Merge remote-tracking branch 'origin/main' into feature/react-18-shor…

606320a

…t-term-support

Merge remote-tracking branch 'origin/main' into feature/react-18-shor…

499d509

…t-term-support

Merge remote-tracking branch 'origin/main' into feature/react-18-shor…

37b3374

…t-term-support

Add deploy to feature validation apps

e08792c

JamesBurnside had a problem deploying to staging May 23, 2023 19:53 — with GitHub Actions Failure

JamesBurnside had a problem deploying to staging May 23, 2023 19:54 — with GitHub Actions Failure

JamesBurnside had a problem deploying to staging May 23, 2023 20:09 — with GitHub Actions Failure

change deploy env

b31eb43

JamesBurnside had a problem deploying to staging May 23, 2023 20:35 — with GitHub Actions Failure

JamesBurnside had a problem deploying to production May 23, 2023 20:35 — with GitHub Actions Failure

JamesBurnside had a problem deploying to staging May 23, 2023 20:36 — with GitHub Actions Failure

JamesBurnside temporarily deployed to production May 23, 2023 20:49 — with GitHub Actions Inactive

JamesBurnside had a problem deploying to staging May 23, 2023 20:58 — with GitHub Actions Failure

JamesBurnside had a problem deploying to production May 23, 2023 20:58 — with GitHub Actions Failure

JamesBurnside had a problem deploying to staging May 23, 2023 20:58 — with GitHub Actions Failure

JamesBurnside requested review from mgamis-msft and prabhjot-msft May 23, 2023 23:13

PorterNan reviewed May 24, 2023

View reviewed changes

emlynmac reviewed May 24, 2023

View reviewed changes

Merge branch 'main' into feature/react-18-short-term-support

8ddc4e2

dmceachernmsft reviewed May 24, 2023

View reviewed changes

PorterNan reviewed May 24, 2023

View reviewed changes

dmceachernmsft reviewed May 24, 2023

View reviewed changes

PorterNan reviewed May 24, 2023

View reviewed changes

dmceachernmsft reviewed May 24, 2023

View reviewed changes

PorterNan approved these changes May 24, 2023

View reviewed changes

dmceachernmsft approved these changes May 24, 2023

View reviewed changes

Merge branch 'main' into feature/react-18-short-term-support

a9b6c1b

emlynmac approved these changes May 24, 2023

View reviewed changes

JamesBurnside merged commit b618b2a into main May 24, 2023
30 checks passed

JamesBurnside deleted the feature/react-18-short-term-support branch May 24, 2023 23:51

JamesBurnside added a commit that referenced this pull request May 25, 2023

Merge React 18 feature branch (#3050)

28cc8de

JamesBurnside mentioned this pull request May 25, 2023

Cherry-pick react 18 support to 1.5.1-beta.5 release branch #3099

Merged

		"build:watch": "",
		"copy-northstar": "cpx \"../northstar-wrapper/dist/dist-esm/*/\" dist/dist-esm/northstar-wrapper/src --clean",

Merge React 18 feature branch #3050

Merge React 18 feature branch #3050

Conversation

JamesBurnside commented May 15, 2023 • edited

What

Why

How Tested

github-actions bot commented May 15, 2023

github-actions bot commented May 15, 2023 • edited

Calling bundle size is decreased✅.

github-actions bot commented May 15, 2023 • edited

CallWithChat bundle size is decreased✅.

github-actions bot commented May 15, 2023 • edited

Chat bundle size is increased❗.

github-actions bot commented May 19, 2023

github-actions bot commented May 23, 2023

github-actions bot commented May 23, 2023

github-actions bot commented May 23, 2023

github-actions bot commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesBurnside May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesBurnside May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesBurnside May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesBurnside May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesBurnside May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlynmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesBurnside May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesBurnside May 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesBurnside commented May 15, 2023 •

edited

github-actions bot commented May 15, 2023 •

edited

github-actions bot commented May 15, 2023 •

edited

github-actions bot commented May 15, 2023 •

edited

JamesBurnside May 24, 2023 •

edited

JamesBurnside May 24, 2023 •

edited

JamesBurnside May 24, 2023 •

edited

JamesBurnside May 24, 2023 •

edited

JamesBurnside May 24, 2023 •

edited

JamesBurnside May 24, 2023 •

edited

JamesBurnside May 24, 2023 •

edited