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

Change options.html to React/Redux #1387

Merged
merged 11 commits into from Dec 23, 2019

Conversation

siefkenj
Copy link
Collaborator

@siefkenj siefkenj commented Dec 4, 2019

This PR changes options.html to use React/Redux. It also serves as an example of how to make components that can be developed/debugged in the browser.

An es-modules directory was created that contains ui.js, which exports React and friends (which are presumed to be loaded as global variables) and thunderbird-compat.js, which mocks the Thunderbird API calls needed to read and set preferences. If the native browser object is detected, thunderbird-compat.js will do nothing.

There's a new build script, build-dev-html.sh which will build and serve the project on localhost:8126. Navigating to http://localhost:8126/options.html you can see options.html rendered in the browser and can use the React/Redux dev tools to debug it. Additionally, all files are watched for changes and automatically recopied/recompiled. Separating components enough that they can be run in the browser should allow for easier testing of individual components.

This PR also includes Redux Toolkit (hopefully this is compatible with AMO!) which simplifies Redux development.

@Standard8
Copy link
Collaborator

Hi, I'm sorry I've not got to this yet. I'm hoping to get time on Thursday.

@siefkenj
Copy link
Collaborator Author

No worries. I've been piling on the code, unsolicited :-P.

@siefkenj
Copy link
Collaborator Author

Have you had any time to look at this? I've got some other ideas that build on it.

Copy link
Collaborator

@Standard8 Standard8 left a comment

Choose a reason for hiding this comment

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

Sorry, I started it on Thursday and didn't get very far.

Here's some of the more general architecture comments. I'd like to dig into the code a little bit more, hopefully I'll get time this evening.

addon/options.html Outdated Show resolved Hide resolved
addon/content/es-modules/thunderbird-compat.js Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
addon/content/es-modules/thunderbird-compat.js Outdated Show resolved Hide resolved
addon/options.html Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@siefkenj
Copy link
Collaborator Author

siefkenj commented Dec 16, 2019

I found a way to hack around the UMD module definitions by adding modules-compat.js. Now react, etc can be imported by the module system. Hopefully my workaround makes sense!

@Standard8
Copy link
Collaborator

I found a way to hack around the UMD module definitions by adding modules-compat.js. Now react, etc can be imported by the module system. Hopefully my workaround makes sense!

Ah interesting, I hadn't realised that React didn't support that yet, I just found facebook/react#14635 and facebook/react#15037 that will hopefully mean they'll support it sometime soon. In the meantime, the work around is probably good enough.

I'll try and take a look at the rest of this some time this week.

Copy link
Collaborator Author

@siefkenj siefkenj left a comment

Choose a reason for hiding this comment

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

There's a bit more hacking needed to get tests to run. The workaround I made imitates the functionality of an AMD module. Since UMD first tries CJS, which is supported by node, attempting to run this code in node results in errors. The best workaround I can think of is to modify the code of the vendor modules to disable the global variable, which will force the UMD module loader to fall back to the AMD mechanism.

@siefkenj
Copy link
Collaborator Author

Any updates on the merge status of this? I almost have a PR that implements tests. It builds on this one and would probably be easier to understand once this PR is merged. I will be away from computers Dec 24-Jan 5.

Copy link
Collaborator

@Standard8 Standard8 left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, this is looking good. I'm sorry I haven't had time to look at this in depth earlier - I've been struggling with getting work done and getting ready for Christmas.

I think we're not far off now, just a few things to address.

build-dev-html.sh Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
addon/content/es-modules/modules-compat.js Outdated Show resolved Hide resolved
addon/content/es-modules/modules-compat.js Outdated Show resolved Hide resolved
addon/prefs.js Outdated Show resolved Hide resolved
addon/content/es-modules/thunderbird-compat.js Outdated Show resolved Hide resolved
addon/options.jsx Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
scripts/build-dev-html.sh Show resolved Hide resolved
scripts/build-dev-html.sh Show resolved Hide resolved
Copy link
Collaborator

@Standard8 Standard8 left a comment

Choose a reason for hiding this comment

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

Thank you for the updates. This looks good, I generally like where it is headed, and thank you for sticking with it.

@Standard8 Standard8 merged commit e2ba155 into thunderbird-conversations:master Dec 23, 2019
@Standard8
Copy link
Collaborator

@siefkenj I've just pushed a829415 which updates the React modules to the latest version, which includes the UMD fix facebook/react#15037 - so I think we might be able to simplify some of these imports now?

Though not entirely sure if we need to wait for the other modules to be updated as well.

@siefkenj
Copy link
Collaborator Author

siefkenj commented Mar 2, 2020

I think it's easiest to wait for all modules to get updated to accept native loading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants