Remove react and add preact #13608
Remove react and add preact #13608
Conversation
src/main.js
Outdated
|
|
||
| // The file system implementation. Change this value to use different | ||
| // implementations (e.g. cloud-based storage). | ||
| "fileSystemImpl" : "filesystem/impls/appshell/AppshellFileSystem" | ||
| }, | ||
| packages: [ |
There was a problem hiding this comment.
We should probably add preact as an node_module instead of inlining it to the thirdparty folder as we have been doing with new dependencies. (related to #12006)
There was a problem hiding this comment.
@petetnt Actually react is being replaced with preact to remove facebook dependencies due to the PATENT issues. But preact-compat uses prop-types which is by facebook and we do not use proptypes in the FileTreeView code, so I removed it manually from preact-compat. So only we are inlining it to the thirdparty folder.
There was a problem hiding this comment.
Fair enough, wasn't aware that compat uses prop-types package as-is
src/project/FileTreeView.js
Outdated
| @@ -31,8 +31,8 @@ | |||
| define(function (require, exports, module) { | |||
| "use strict"; | |||
|
|
|||
| var React = require("thirdparty/react"), | |||
| ReactDOM = require("thirdparty/react-dom"), | |||
| var React = require("thirdparty/preact"), | |||
There was a problem hiding this comment.
We probably should just find-and-replace all the React and ReactDOM mentions to Preact
test/spec/FileTreeView-test.js
Outdated
| _ = require("thirdparty/lodash"); | ||
|
|
||
| // Preact Test Utils doesn't have findRenderedDOMComponentWithTag method | ||
| // So create it | ||
| RTU.findRenderedDOMComponentWithTag = function(root, tagName) { |
There was a problem hiding this comment.
Instead of extending the RTU object can we just add findRenderedDOMComponentWithTag as a helper method?
function findRenderedDOMComponentWithTag(root, tagName) { ... }
...
findRenderedDOMComponentWithTag(...,...)Just in case findRenderedDOMComponentWithTag gets added to RTU in future
|
Nice work @boopeshmahendran! |
|
@petetnt Thanks for reviewing. I'll address these comments. |
|
I would really like to highlight the fact that @petetnt had actually suggested to use preact which resulted in minimal code changes. Great work @petetnt and @boopeshmahendran 👍 . |
|
https://github.com/mikaeljorhult/brackets-todo uses React through var React = brackets.getModule('thirdparty/react');
var ReactDOM = brackets.getModule('thirdparty/react-dom');with this PR it will be broken. |
|
@ficristo I can send a PR there later |
|
@ficristo @petetnt I have raised a PR mikaeljorhult/brackets-todo#145 at https://github.com/mikaeljorhult/brackets-todo changing react to preact |
|
I was wondering if instead we should have handled this thing in core with some path mapping through RequireJS and possibly with some warnings. I noticed BracketsTODO is broken because I use it, but maybe there are other extensions. If I remember correctly for other libs we use the path |
No description provided.