Open
Bug 1216657
Opened 9 years ago
Updated 2 years ago
Use react-dev based off of build flag, and work in all loaders
Categories
(DevTools :: General, defect)
Tracking
(Not tracked)
NEW
People
(Reporter: jsantell, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
4.41 KB,
patch
|
Details | Diff | Splinter Review |
PropTypes are only enforced when using the dev build. We should do this in testing to ensure our model validations are working in tests.
Reporter | ||
Updated•9 years ago
|
Summary: Use react-dev when testing → Use react-dev based off of build flag, and work in all loaders
Reporter | ||
Comment 1•9 years ago
|
||
r? jryans for loader changes, r? janx for about:debugging changes
Attachment #8676572 -
Flags: review?(jryans)
Attachment #8676572 -
Flags: review?(janx)
Reporter | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0080663c042
Comment 3•9 years ago
|
||
Comment on attachment 8676572 [details] [diff] [review] 1216657-react-path.patch Review of attachment 8676572 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/shared/Loader.jsm @@ +102,5 @@ > // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠ > // Allow access to xpcshell test items from the loader. > + "xpcshell-test": "resource://test", > + // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠ > + "react": AppConstants.DEBUG_JS_MODULES ? This should probably also return react-dev when normal DEBUG.
Reporter | ||
Comment 4•9 years ago
|
||
It's my understanding that DEBUG_JS_MODULES is the JS version of `DEBUG`, and if we do things if either is set on, then that defeats the purpose of it right?
Reporter | ||
Comment 5•9 years ago
|
||
That is, DEBUG_JS without DEBUG makes sense otherwise everything's super slow, but DEBUG without DEBUG_JS doesn't make sense to me
Comment 6•9 years ago
|
||
Comment on attachment 8676572 [details] [diff] [review] 1216657-react-path.patch Review of attachment 8676572 [details] [diff] [review]: ----------------------------------------------------------------- Doesn't break about:debugging so fine by me.
Attachment #8676572 -
Flags: review?(janx) → review+
Comment on attachment 8676572 [details] [diff] [review] 1216657-react-path.patch Review of attachment 8676572 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to keep the "exact path" module ID "devtools/client/shared/vendor/react" until we have a real solution for loading vendor modules similar to Node, etc. Once such a thing does exist, we can rejoice and use short names if we want. I'll keep thinking about ideas for this in bug 1201710. ::: devtools/shared/Loader.jsm @@ +102,5 @@ > // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠ > // Allow access to xpcshell test items from the loader. > + "xpcshell-test": "resource://test", > + // ⚠ DISCUSSION ON DEV-DEVELOPER-TOOLS REQUIRED BEFORE MODIFYING ⚠ > + "react": AppConstants.DEBUG_JS_MODULES ? I don't feel strongly, but it seems nice to let the flags be independent, so you have more control of what's happening. We can always tweak which flags are consulted, so I am not too worried. @@ +150,5 @@ > let acornURI = this.fileURI(OS.Path.join(sharedDir, "acorn")); > let acornWalkURI = OS.Path.join(acornURI, "walk.js"); > let sourceMapURI = this.fileURI(OS.Path.join(sharedDir, > "sourcemap", "source-map.js")); > + let reactURI = this.fileURI(OS.path.join(devtoolsDir, "shared", "vendor", The "client" path segment is missing here.
Attachment #8676572 -
Flags: review?(jryans) → review-
Reporter | ||
Comment 8•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > Comment on attachment 8676572 [details] [diff] [review] > 1216657-react-path.patch > > Review of attachment 8676572 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd like to keep the "exact path" module ID > "devtools/client/shared/vendor/react" until we have a real solution for > loading vendor modules similar to Node, etc. Once such a thing does exist, > we can rejoice and use short names if we want. I'll keep thinking about > ideas for this in bug 1201710. So just have "devtools/client/shared/vendor/react" either map to prod or dev version? > @@ +150,5 @@ > > let acornURI = this.fileURI(OS.Path.join(sharedDir, "acorn")); > > let acornWalkURI = OS.Path.join(acornURI, "walk.js"); > > let sourceMapURI = this.fileURI(OS.Path.join(sharedDir, > > "sourcemap", "source-map.js")); > > + let reactURI = this.fileURI(OS.path.join(devtoolsDir, "shared", "vendor", > > The "client" path segment is missing here. Huh, I guess I was only testing (and on try too?) the other branch. I don't quite get the different "providers" here.
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > > Comment on attachment 8676572 [details] [diff] [review] > > 1216657-react-path.patch > > > > Review of attachment 8676572 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I'd like to keep the "exact path" module ID > > "devtools/client/shared/vendor/react" until we have a real solution for > > loading vendor modules similar to Node, etc. Once such a thing does exist, > > we can rejoice and use short names if we want. I'll keep thinking about > > ideas for this in bug 1201710. > > So just have "devtools/client/shared/vendor/react" either map to prod or dev > version? Right. That's how browser loader handled the situation before. > > @@ +150,5 @@ > > > let acornURI = this.fileURI(OS.Path.join(sharedDir, "acorn")); > > > let acornWalkURI = OS.Path.join(acornURI, "walk.js"); > > > let sourceMapURI = this.fileURI(OS.Path.join(sharedDir, > > > "sourcemap", "source-map.js")); > > > + let reactURI = this.fileURI(OS.path.join(devtoolsDir, "shared", "vendor", > > > > The "client" path segment is missing here. > > Huh, I guess I was only testing (and on try too?) the other branch. I don't > quite get the different "providers" here. There is currently no real testing of the SrcdirProvider, aside from ensuring it has the correct number of path lines in it (but they can still be invalid, probably a test should check that at some point). It's only used if you set a "srcdir" path to load files directly from your source tree, instead of from a fully packaged Firefox. This is not a default setup, so currently you only go down this path if you went out of your way to turn it on. The idea is you can use this plus the DevTools reload shortcut :ochameau added (Cmd-Opt-R) to see changes faster. There are still a few sharp edges with it, so I believe it has no current users. We plan to make some noise on the list about it soon though after some more polishing.
Reporter | ||
Comment 10•9 years ago
|
||
This got much simpler. Jan, no changes in aboutdebugging anymore.
Attachment #8676572 -
Attachment is obsolete: true
Attachment #8677563 -
Flags: review?(jryans)
Comment on attachment 8677563 [details] [diff] [review] 1216657-react-path.patch Review of attachment 8677563 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable, but I think browser loader's check can still be removed. ::: devtools/shared/Loader.jsm @@ +151,5 @@ > let acornWalkURI = OS.Path.join(acornURI, "walk.js"); > let sourceMapURI = this.fileURI(OS.Path.join(sharedDir, > "sourcemap", "source-map.js")); > + let reactURI = this.fileURI(OS.path.join(devtoolsDir, "client", "shared", "vendor", > + AppConstants.DEBUG_JS_MODULES ? "react-dev.js" : "react")); I believe this should be "react.js" here, not just "react".
Attachment #8677563 -
Flags: review?(jryans)
Reporter | ||
Comment 12•9 years ago
|
||
Going in bug 960776 is fixing the browser loader to use react/react-dev correctly (not pointing at right address right now), with a note saying that this bug should get rid of it and move it to the Loader proper. This is needed for TestUtils, so once that lands, we'll atleast have a test requiring it to prevent regressions
Reporter | ||
Updated•8 years ago
|
Assignee: jsantell → nobody
Status: ASSIGNED → NEW
Updated•8 years ago
|
Whiteboard: [platform-rel-Facebook][platform-rel-ReactJS]
Updated•8 years ago
|
platform-rel: --- → ?
Updated•8 years ago
|
platform-rel: ? → ---
Whiteboard: [platform-rel-Facebook][platform-rel-ReactJS]
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•