search

OfficeDev / Office-Addin-TaskPane-React

Template to get start started writing a TaskPane Office Add-in for the React framework using TypeScript
Other
54 stars 35 forks source link

Replace react-hot-loader with new fast refresh #90

Closed sjwilczynski closed 7 months ago

sjwilczynski commented 2 years ago

As stated in the react-hot-loader readme if possible it should be replaced with React Fast Refresh. The easiest way to integrate in an app using webpack is to utilize webpack plugin. In scope of this PR, we adjust the existing webpack config and babel setup to be able to utilize the mentioned plugin. Here is the gif showing hot reloading with the changes:

fastRefresh

I can create same PR for JS version of the template but want to get feedback first :)

igor-ribeiiro commented 2 years ago

/Azurepipelines run

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
igor-ribeiiro commented 2 years ago

@sjwilczynski could you please try running npm run test? Our pipelines are getting timeout errors (the tests never finish). As for the PR, I have no problem using this framework given it is the recommended step.

sjwilczynski commented 2 years ago

Will do, there are actually some changes I should make in tests, thanks for bringing my attention to that!

BTW shouldn't azure pipeline be run automatically? I saw you added a comment to start them

sjwilczynski commented 2 years ago

@igor-ribeiiro I realized that running these e2e tests might not be super easy to run externally. It needs windows and I needed to manually choose few things in the terminal. Maybe that's why the tests hanged on Azure Pipelines?

Anyway, here is output of running npm test:

PS D:\ubuntu\programy\github\Office-Addin-TaskPane-React> npm test

> office-addin-taskpane-react@0.0.1 test
> npm run test:unit && npm run test:e2e

> office-addin-taskpane-react@0.0.1 test:unit
> mocha -r ts-node/register test/unit/*.test.ts

  Excel
The range address was G4.
    ✔ Run

  PowerPoint
    ✔ Run

  Word
    ✔ Run

  3 passing (13ms)

> office-addin-taskpane-react@0.0.1 test:e2e
> mocha -r ts-node/register test/end-to-end/*.ts

  Test Excel Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
(node:17620) Warning: Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification.
(Use `node --trace-warnings ...` to show where the warning was created)
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? No
Starting the dev server... (npm run dev-server -- --config ./test/end-to-end/webpack.config.js )
The dev server is running on port 3000. Process id: 11836
Sideloading the Office Add-in...
Started.
    Get test results for Excel taskpane project
      ✔ Validate expected result count (10222ms)
      ✔ Validate expected result name
      ✔ Validate expected result

  Test PowerPoint Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? No
The dev server is already running on port 3000.
Sideloading the Office Add-in...
Started.
    Get test results for PowerPoint taskpane project
      ✔ Validate expected result count (3908ms)
      ✔ Validate expected result name
      ✔ Validate expected result

  Test Word Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? No
The dev server is already running on port 3000.
Sideloading the Office Add-in...
Started.
    Get test results for Word taskpane project
      ✔ Validate expected result count (5995ms)
      ✔ Validate expected result name
      ✔ Validate expected result

Debugging is being stopped...
Stopped dev server. Process id: 11836
Debugging has been stopped.

  9 passing (32s)

To make closing apps work, I also needed to change the command in test-helpers.ts in line 33 from tskill ${processName} to taskkill /IM ${processName}.exe /F

igor-ribeiiro commented 2 years ago

/Azurepipelines run

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
igor-ribeiiro commented 2 years ago

They should be automatic. I don't remember why it isn't exactly but it had something to do with security reasons, given this is a public repository.

igor-ribeiiro commented 2 years ago

There shouldn't be anything to "choose" while running the tests. What exactly are those decisions? We run tests on Mac and Windows, what is the environment you are testing on?

For clarity, the tests are just hanging and getting a timeout after here:

Test Excel Task Pane Project
You already have trusted access to https://localhost./
Certificate: /Users/testaccount/.office-addin-dev-certs/localhost.crt
Key: /Users/testaccount/.office-addin-dev-certs/localhost.key
(node:80192) Warning: Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification.
(Use `node --trace-warnings ...` to show where the warning was created)
Starting without debugging...
App type: desktop
The dev server is already running on port 3000.
Sideloading the Office Add-in...
Started.
    Get test results for Excel taskpane project
##[error]The operation was canceled.
sjwilczynski commented 2 years ago

So then Azure Pipelines have different issue than I had locally. I am running this on Windows and when I said about "manual decision" when you look at the logs I attached there is for every app (Excel, PowerPoint, Word) line ? Allow localhost loopback for Microsoft Edge WebView? No where I get Yes/No option to choose from - but that might be a setup issue on my machine

akrantz commented 2 years ago

When using the Edge webview, you need to allow localhost loopback before running the tests, since the tests use localhost.

@igor-ribeiiro It would be helpful to add a check (when using the Edge webview) if localhost loopback is allowed or not, and show an error message localhost loopback is not enabled.

@sjwilczynski You can use `npx office-addin-dev-settings appcontainer --loopback" to enable loopback.

millerds commented 2 years ago

@sjwilczynski are you still interested in helping us with this PR?

sjwilczynski commented 2 years ago

Yes sure, do you want me to do something else apart from resolving conflicts? I am not sure why the tests could pass locally and freeze on CI.

millerds commented 2 years ago

Have you tried locally after allowing the loopback? Can you also run it without the test-helpers.ts change you mentioned?

millerds commented 2 years ago

/Azurepipelines run

azure-pipelines[bot] commented 2 years ago
Pull request contains merge conflicts.
millerds commented 2 years ago

Sorry . . . I thought I could updated the branch with the changes from OfficeDev:master myself, but it doesn't look like that worked (your branch has the changes, but it still thinks there's a conflict because I couldn't update your fork master.). If you can resolve the merge conflict and get the branch right again we can run the tests in the pipeline again (looks like the base image has been updated).

igor-ribeiiro commented 2 years ago

/Azurepipelines run

azure-pipelines[bot] commented 2 years ago
Pull request contains merge conflicts.
igor-ribeiiro commented 2 years ago

/Azurepipelines run

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
sjwilczynski commented 2 years ago

I ran npx office-addin-dev-settings appcontainer --loopback --yes .\test\end-to-end\test-manifest.xml as it asked me for path to manifest but in the end it didn't change anything - I still had to manually allow loopack on each test. Here are logs without my change in test-helpers:

PS D:\ubuntu\programy\github\Office-Addin-TaskPane-React> npx office-addin-dev-settings appcontainer --loopback --yes .\test\end-to-end\test-manifest.xml
Loopback is allowed.
PS D:\ubuntu\programy\github\Office-Addin-TaskPane-React> npm run test

> office-addin-taskpane-react@0.0.1 test
> npm run test:unit && npm run test:e2e

> office-addin-taskpane-react@0.0.1 test:unit
> mocha -r ts-node/register test/unit/*.test.ts

  Excel
The range address was G4.
    ✔ Run

  PowerPoint
    ✔ Run

  Word
    ✔ Run

  3 passing (26ms)

> office-addin-taskpane-react@0.0.1 test:e2e
> mocha -r ts-node/register test/end-to-end/*.ts

  Test Excel Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
(node:21232) Warning: Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification.
(Use `node --trace-warnings ...` to show where the warning was created)
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? Yes
Starting the dev server... (npm run dev-server -- --config ./test/end-to-end/webpack.config.js )
The dev server is running on port 3000. Process id: 15732
Sideloading the Office Add-in...
Started.
    Get test results for Excel taskpane project
      ✔ Validate expected result count (29981ms)
      ✔ Validate expected result name
      ✔ Validate expected result
    1) "after all" hook: Teardown test environment and shutdown Excel in "Test Excel Task Pane Project"

  Test PowerPoint Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? Yes
The dev server is already running on port 3000.
Sideloading the Office Add-in...
Started.
    Get test results for PowerPoint taskpane project
      ✔ Validate expected result count (8108ms)
      ✔ Validate expected result name
      ✔ Validate expected result
    2) "after all" hook: Teardown test environment and shutdown PowerPoint in "Test PowerPoint Task Pane Project"

  Test Word Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? Yes
The dev server is already running on port 3000.
Sideloading the Office Add-in...
Started.
    Get test results for Word taskpane project
      ✔ Validate expected result count (10099ms)
      ✔ Validate expected result name
      ✔ Validate expected result
    3) "after all" hook: Teardown test environment and shutdown Word in "Test Word Task Pane Project"

Debugging is being stopped...
Stopped dev server. Process id: 15732
Debugging has been stopped.

  9 passing (2m)
  3 failing

  1) Test Excel Task Pane Project
       "after all" hook: Teardown test environment and shutdown Excel in "Test Excel Task Pane Project":
     Error: Unable to kill Excel process. false
      at Object.<anonymous> (test\end-to-end\src\test-helpers.ts:40:11)
      at rejected (test\end-to-end\src\test-helpers.ts:25:65)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

  3) Test Word Task Pane Project
       "after all" hook: Teardown test environment and shutdown Word in "Test Word Task Pane Project":
     Error: Unable to kill Word process. false
      at Object.<anonymous> (test\end-to-end\src\test-helpers.ts:40:11)
      at step (test\end-to-end\src\test-helpers.ts:52:23)
      at Object.throw (test\end-to-end\src\test-helpers.ts:33:53)
      at rejected (test\end-to-end\src\test-helpers.ts:25:65)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)

with change to how I kill tasks:

PS D:\ubuntu\programy\github\Office-Addin-TaskPane-React> npm run test

> office-addin-taskpane-react@0.0.1 test
> npm run test:unit && npm run test:e2e

> office-addin-taskpane-react@0.0.1 test:unit
> mocha -r ts-node/register test/unit/*.test.ts

  Excel
The range address was G4.
    ✔ Run

  PowerPoint
    ✔ Run

  Word
    ✔ Run

  3 passing (37ms)

> office-addin-taskpane-react@0.0.1 test:e2e
> mocha -r ts-node/register test/end-to-end/*.ts

  Test Excel Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
(node:8832) Warning: Setting the NODE_TLS_REJECT_UNAUTHORIZED environment variable to '0' makes TLS connections and HTTPS requests insecure by disabling certificate verification.
(Use `node --trace-warnings ...` to show where the warning was created)
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? Yes
Starting the dev server... (npm run dev-server -- --config ./test/end-to-end/webpack.config.js )
The dev server is running on port 3000. Process id: 9840
Sideloading the Office Add-in...
Started.
    Get test results for Excel taskpane project
      ✔ Validate expected result count (21532ms)
      ✔ Validate expected result name
      ✔ Validate expected result

  Test PowerPoint Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? Yes
The dev server is already running on port 3000.
Sideloading the Office Add-in...
Started.
    Get test results for PowerPoint taskpane project
      ✔ Validate expected result count (6605ms)
      ✔ Validate expected result name
      ✔ Validate expected result

  Test Word Task Pane Project
You already have trusted access to https://localhost/.
Certificate: C:\Users\Stachu\.office-addin-dev-certs\localhost.crt
Key: C:\Users\Stachu\.office-addin-dev-certs\localhost.key
Starting without debugging...
App type: desktop
? Allow localhost loopback for Microsoft Edge WebView? Yes
The dev server is already running on port 3000.
Sideloading the Office Add-in...
Started.
    Get test results for Word taskpane project
      ✔ Validate expected result count (9536ms)
      ✔ Validate expected result name
      ✔ Validate expected result

Debugging is being stopped...
Stopped dev server. Process id: 9840
Debugging has been stopped.

  9 passing (2m)

I am not sure if it makes any difference but locally I am using Windows 11.

Also now I see that Win10 and Mac pipelines work fine but the other two timeout: image

millerds commented 2 years ago

The other two are running a command that changes the type of webview being used by the office hosts. Unless you run the same commands locally (npx office-addin-dev-settings webview ), it will use the default one that passes.

millerds commented 2 years ago

I just tried your branch locally and was able to repro the problem. When I run the test the task pane comes up blank (i.e. it's not loading any taskpane content). The same thing happens when I run "npx office-adding-dev-settings webview manifest.xml edge-legacy" and then run "npm start". So there is something with the PR the is having trouble with older (though still supported in certain versions of office) webview technologies (EdgeHtml and IE webviews).

sjwilczynski commented 2 years ago

Hmmm this is interesting. I would love to fix this (or at least try :)) but I have a problem with reproing the issue locally. I tried running npx office-addin-dev-settings webview manifest.xml ie && npx office-addin-debugging start manifest.xml --no-debug and still works without a problem, similar with edge-legacy. Could you show me the exact command you ran the reproduce the issue? :)

igor-ribeiiro commented 2 years ago

Hmmm this is interesting. I would love to fix this (or at least try :)) but I have a problem with reproing the issue locally. I tried running npx office-addin-dev-settings webview manifest.xml ie && npx office-addin-debugging start manifest.xml --no-debug and still works without a problem, similar with edge-legacy. Could you show me the exact command you ran the reproduce the issue? :)

Can you try running npm run test? I believed the error was there last time I checked.

millerds commented 2 years ago

You should run plain 'npm start' as that is what the test is going. Although the error is not specific to tests when I tried it. I just ran 'npx office-addin-dev-settings webview manifest.xml edge-legacy' (note there was a typo when I mentioned it before) followed by 'npm start' and got the repro. I also verified that the webview being used was the edge-legacy webview by checking the "security settings" in the task pane menu (the '<' when you move the mouse over the task pane) says "Microsoft Edge Legacy".

I also just remembered that in order to set the webview type being use you have to be in pre-release channel of Office or the setting is ignored. To do this:

  1. Open Excel
  2. Click "File > Account"
  3. Click "Update Channel"
  4. Make sure "I will manage3 my Office update channel" is clicked and then choose "Current Channel (Preview)" in the channel drop down
  5. Agree to the terms and conditionsand click OK

Once office finishes updating its bits you should see the repro. You can always switch back to the channel you were on previously when you are done.

Edward-Zhou commented 1 year ago

@sjwilczynski Thank you for your efforts on creating Office AddIn Taskpane React without react-hot-loader.

I tried to download sjwilczynski/Office-Addin-TaskPane-React, after installing the packing and start the project, I got error like image

In fact, I am trying to add react router to default excel addin template, but I hit issue about react-hot-reloader.

Do you have any thoughts about enable react router in excel task pane?

Thanks.

Rick-Kirkham commented 12 months ago

We will be updating the template to use React 18 which includes react-hot-loader. Does this obviate the need to switch to Fast Refresh?

millerds commented 7 months ago

Updates to template cover this.