Update React version, move to JSX, modularize and refactor roughly everything... #45
Conversation
…ce it's adding globals everywhere
|
When running "npm install" I get: npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.1.3 (node_modules\fsevents): However, "npm run dev" seems to work OK - but I've now got a local change to package-lock.json which removes fsevents. What is that package used for? |
|
It's used by webpack for file watching on macOS. It's annoying that it shows up as a warning on windows, but I don't think we can do anything about that. :( |
What concerns me more is that it leaves an uncommited change in my tree, which at some point is going to trip me over. |
|
Thanks! For some reason the change I have left over (after upgrading npm) is:
which seems odd as the addon still WFM. We can deal with that later. |
I've been poking around with this for the last two weekends and occasionally after work last week. The code for about:sync is hard to maintain and has grown somewhat gross over time. This fixes that. I had been planning to do it for quite some time, but the most recent addition of the server editor and you asking about making it more modular convinced me it was worthwhile.
It's a huge set of patches that touch nearly every line of code, but many of them are mechanical changes. I suspect you might be better off trying it locally instead of doing a thorough review, but it's your project and your call.
The summary of what this PR does is:
Change all the code to use JSX instead of explicit React.createElement calls.
This makes things much easier to read (IMO), and is in line with what e.g. other Mozilla react projects use (for example https://github.com/devtools-html/perf.html). We use babel to compile the JSX into React.createElement calls, but for nothing else -- it leaves in modern JS features.
Update the version of dependencies. The big update here is updating to React v16.
Related: we don't use react-simpletabs any more, and chose to roll our own, mainly since react-simpletabs isn't compatible with v16 (it's master branch has a couple commits that supposedly add this, but it hasn't been published to NPM, and has a number of bugs around the compatibility. Given that the set of features we use from it takes only 50ish lines to implement, I just did that).
Use webpack to bundle dependencies instead of vendoring them in this repo.
Webpack also takes care of JSX => JS and many files => one file. For dependencies with CSS (which we don't have any right now), we can modify the webpack config to bundle that as well, but I haven't done that (I'd like to eventually replace our table view with something like
react-table, so I've tried it out).The code doesn't use
Cu.importdirectly any more.Now that this is more modularized, it was annoying that
Cu.importis a global import, e.g. one file would haveCu.importand it would apply to all files imported after theCu.import, which is confusing and error prone.Instead we do
let { Blah } = importLocal("resource://path/to/whatever"), whereimportLocalis a helper that uses the two-argumentCu.importunder the hood. It also helps be explicit when we're using something that wouldn't be available to e.g. an embedded webextension, or whatever this will become in the future.The code for validation is now much improved, and consolidated.
Validation problem reports that are common between Bookmarks and other validators now use the same code. It also has a couple new features, for example, clicking on the dotted underline IDs will show a modal.
Additionally, when there's a validation problem type we don't have display code for, it will report it and show it in a table view. This showed me that we didn't have display code for deletedChildren prior to this (although we do now).