-
Notifications
You must be signed in to change notification settings - Fork 34
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
Upgrade to React18, Fluent9, functional components, hooks #128
Upgrade to React18, Fluent9, functional components, hooks #128
Conversation
Does this also resolve the webpack warning that the bundle size is too large? I had a change a while ago that was going to fix that, but I was having trouble with the tests. Not sure if this update does anything to help that. |
No. I didn't try to do anything about the bundle size in this PR (except that a side effect of all these changes is that there are about 28,000 fewer characters in the project). That will have to be a future PR. I think that having a separate "vendors" bundle is only helpful if we also use the "optimization" property of webpack. Something like the following:
But I have not investigated this yet. |
I tried running the project as is (it should be runnable without executing the convert script) and the insert text didn't work. |
I think the '*-office-document.ts' files belong in the taskpane folder if they relate to the taskpane code. However, I'm a little confused by the use of the methods before the conversion vs. after the conversion. |
I get the same result, but that doesn't surprise me. The TextInsertion.tsx that you are running when you do this is written on the assumption that it is being run in an end2end test. The But I don't see why this is a problem. Why should it be runnable outside of the tests but without executing the convert script? The emitted projects work and the end2end tests work. Why is that not enough? |
It's not a big deal, but I think the best practice is to have the custom logic that calls Office.js outside any folder that is for a particular feature (or shape), like a task pane, because the custom logic could be called in some other shape too. |
Ricky the taskpane/* code can have office.js calls in it. The folder structure was to reinforce separation between the regular add-in UI code in the task pane, versus custom functions code, or command code which can have different limitations. I'd not personally a fan of "office-document" as a name. I think the key thing is that it's an example of how the add-in does basic code. The other templates have this named simply as "excel.ts", "word.ts", etc. |
Is more for the benefit of maintaining the repo. If I'm not making changes to the convert script it's more convenient to be able to open the repo, update the files and run it without having to run the convert script (remembering to undo those changes before checking in) and debug or verify my in-progress changes. This also why there is a default "manifest.xml" file in the repo that default to one host. |
That's a fair point when code is actually being shared between features/shapes. If I have a large project I'm not going to put all my office.js interaction separate from the feature just in case it might get used somewhere else. I would put intentionally shared code in a shared location and keep code only being used by one part with that code. Since this is template and not a developed project it's hard to say where developers will take it, but it's not obvious that this file and code are going to be shared since there is nothing to share it with. |
I moved them to the taskpanes folder. |
I fixed this. It seems to work now. |
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.
When looking at the test code in VSCode I was seeing a bunch of linter errors (mostly around prettier). Since it's test code we remove on deployment it's not as critical we be as clean, but it's something to consider just to be in a clear habit of doing that everywhere.
I also noticed that only one of the CI tests is passing (and it's been consistently like this with each commit). Need to get them all passing before checking in. I can help you learn how to do this if you want.
I'm not seeing that. Can you give me an example or a screenshot? When I run |
Yes. I've been waiting till we got consensus about the content to bring that up. As I mentioned in the PR, it's not surprising that the two old webview tests are failing since React 18 and Fluent 9 don't support them. I wouldn't mind just removing those tests for the two React templates. Alternatively, we could replace them with a test of the "fail fast" JavaScript that I put in the taskpane.html:
What would be your preference? I don't know what the Mac failure could be. Did you tell me once that sometimes you just need to restart the Mac? |
As I understand it the updated react and fluent doesn't work with the IE control, but I thought it should still work with WebView v1 (I always get the pre-release names mixed up) as well as WebView2 (which is passing). I could be wrong on that, but it would be good to confirm (I did track down the changes that broke IE webview compatibility before). Disabling the test for IE is one way to fix at least that part (I think I've reluctantly done it somewhere else), but I think it would be nice to have tests that confirm the fail fast message (I thinks that is the type of thing that store submissions are checking for now). On occasion I've had mac failures that were a result of the pipeline machines needing attention, but I've recently checked them, and I believe they're OK. This smells like something to investigate. I might be able to help with that given the machines resources I have. |
According to this page, Fluent V9 doesn't support Edge Legacy (EdgeHTML): https://github.com/microsoft/fluentui/wiki/Browser-Support |
I see . . . looks like there are two different packages now @fluentui/react which is version 8 and @fluentui/react-components which is version 9 and you have not just updated to a current version of react and @fluentui/react but converted to the brand new @fluentui/react-components. As I understand it, Edge legacy is still supported by @fluentui/react and for @fluentui/react-components it's not explicitly supported, but it's not necessarily broken either. I think it would be nice to understand the specific incompatibility between our code w/ react-components and EdgeHTML. I do worry about giving our customers a starting point that is potentially incompatible with some supported version of office (https://learn.microsoft.com/en-us/office/dev/add-ins/concepts/browsers-used-by-office-web-add-ins). While the store does recommend (and test) these older configurations, the don't require them to work so maybe it's OK (https://learn.microsoft.com/en-us/office/dev/add-ins/develop/support-ie-11?tabs=ie). There should at least be documentation helping customers convert the code if they have requirement to support older versions (and configurations) of office. Either way we should get the tests to do the right thing. |
The plan is to save the current React project(s) as samples in our samples repo. Then documentation will point people who need to support the older webviews to the samples. |
Change Description:
Use React ver. 18
Remove all react-hot-loader stuff because it is now built into React 18.
Use functional components instead of class components and use React effects instead of lifecycle events for state management.
Use Fluent React UI v9.
Use the Fluent V9 "makeStyles" API for styling.
Removed the unused "Commands" files and manifest markup. REVERSED THIS 10/3/23.
Remove the testing of whether Office is initialized. It served no purpose.
Change the functionality of the resulting projects so that they show real world React state management. They all now insert text into the document.
Add check in taskpane.html for Trident (IE) and Edge Legacy and gracefully fail if positive. I WOULD EXPECT THE IE AND EDGE LEGACY TESTS TO FAIL FOR THIS REASON. WE SHOULD PROBABLY JUST NOT HAVE THOSE TESTS FOR THIS TEMPLATE.
Enforce Separation of Concerns by moving code that calls Office.js out of the React component modules and into their own modules.
Update all unit and end2end tests to account for the above.
Use the actual production *.tsx component files for the end2end tests instead of a separate set of (near, but not exact) duplicate files.
Update convertToSingleHost.js to account for all of the above.
Remove code and files that would only be relevant if we were including tests in the emitted projects.
Do these changes impact any npm scripts commands (in package.json)? (e.g., running 'npm run start')
If Yes, briefly describe what is impacted.
No
Do these changes impact VS Code debugging options (launch.json)?
If Yes, briefly describe what is impacted.
No
Do these changes impact template output? (e.g., add/remove file, update file location, update file contents)
If Yes, briefly describe what is impacted.
Yes. See Change Description above.
Do these changes impact documentation? (e.g., a tutorial on https://docs.microsoft.com/en-us/office/dev/add-ins/overview/office-add-ins)
If Yes, briefly describe what is impacted.
Yes. The quickstart/tutorial for a React-based add-in will have to be changed and possibly other files too. THIS PR SHOULD NOT BE MERGED TO PRODUCTION UNTIL THE DOC REVISIONS ARE READY TO BE MERGED TOO. NEEDS TO HAPPEN AT THE SAME TIME.
If you answered yes to any of these please do the following:
> Include 'Rick-Kirkham' in the review
> Make sure the README file is correct
Validation/testing performed:
All unit and end2end tests work locally.
All emitted projects tested and they all work.