-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
Add basic testing page for browser quicks #8589
Conversation
<script src="../../build/react.js"></script> | ||
<script src="../../build/react-dom.js"></script> | ||
|
||
<script src="../react-loader.js"></script> |
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.
I've only done it for the basic example, but I could imagine doing this everywhere. We could also add an extra flag to pull in Babel.
<option value="15.3.0">15.3.0</option> | ||
<option value="15.2.1">15.2.1</option> | ||
<option value="15.1.0">15.1.0</option> | ||
</select> |
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.
Totally arbitrary and temporary list. What's a good way to pull in all of the version data?
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.
It would be nice if unpkg or npm had a small API to get this information, but it doesn't look like they do. The easiest way may be to just manually keep it up to date as part of the release process. It would be nice to have something like CHANGELOG but in JSON.
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.
@nhunzaker If you make a request to the NPM registry and parse out the version keys, you should be able to get a similar output to the npm view react versions
command. Here is the response on the registry for react, it contains all versions published. (Note: You could fall back to Yarn if NPM was down.)
EDIT: If you are going to be doing all of this in the browser, that method may actually fail, as the NPM registry turned off CORS support. An alternative option would be to fetch the tags instead.
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.
@vinnymac Very cool. I think tags is precisely what I want.
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.
Added. We'll pull from the Github API, though this fails in IE9 because of silliness with CORS and XDomainRequest. Something to figure out later...
Otherwise, this page looks good and works down to IE9.
I think I can just push changes to your branch, as long as you didn't uncheck that option when opening the PR. |
This is a good start 👍 How are we going to structure things at a high-level? Should tests be grouped by together in separate pages ( Personally, I think separate pages is the way to go since I expect this to get pretty large by the time we're done. If we do use separate pages, does the |
Woah neat. Good deal. I think we should do separate pages too. We could add a dropdown just like React versions. Maybe setup a script that generates it (or do it by hand for a while). Another part of this is documentation. Where does documentation for fixes to random browser issues live? |
1a09655
to
c72c949
Compare
Updated the readme in |
@gaearon mentioned renaming |
IE Edge | ||
Chrome | ||
Firefox | ||
Safari |
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.
Curiously, I couldn't find anything about base line browser support for React. Would it be the same as Facebook? Either way, do you know what this support matrix looks like?
Otherwise, drawing from the list of ES5 compliant browsers makes the most sense to me:
http://caniuse.com/#search=ES5
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.
@nhunzaker Here is what the docs says about Browser Support. Having ES5 does appear to be the boundary. Shimming support for other browsers is possible (IE8 on React v0.14).
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.
Ah... neat. Thank you!
bf878e2
to
b78ad42
Compare
Excited to see work on this. Let me know if you get blocked on something! |
@gaearon you're fast! I was just writing this up.. Added:
I also wrote up a section on At this point, I feel good about everything. The only outstanding issues relates to fetching the version information in IE9 for the dropdown menu. I think a next step would be to catalog past issues. I can think of three off the top of my head:
Though I'm sure there are more. Still, I think these are additive. At some point, I would love to check this in so that others can contribute to it more easily. |
And... just fixed the lint issues. Hopefully CI Passes now. |
@nhunzaker I added a small commit to show the values for the controlled I think we need to decide on some high-level design structure before adding anything else. We want to have an accessible set of test cases that anyone can run through. It should be easy enough for a new contributor to intuitively follow. IMO the clearest way to do that is to have instructions inline. The advantages with this approach is that it makes it very, very easy for a contributor to open this page and work from a central place to test their changes. We could even have little checkboxes that people can use to track which tests they've done every time their refresh/check changes. The downside is that our component structure increases in complexity. Each individual test case would likely need to maintain its own state and all that. If we did it this way, we'd likely want to start thinking about a better structure (instead of a bunch of inlined components) Alternatively we could have them listed in each individual README. The advantage there is that its probably easier to maintain and easier to read over. The downside is that it makes it slightly harder to track what you're doing and requires you to have two pages open at once. What do you think? |
I like the first approach. Especially if we can make it available online. Each file would be a stand alone issue reproducer, making it easier to produce and avoids possibly missing context. So let's start making cases? |
Should we move away from loading static HTML files and provide a server that users can run? It would make future deployment efforts easier. Then we could actually separate stuff out into different files and organize a bit better. And I think we'd sidestep CORS issue too since we can just request version history from the server. Then users can just do |
I feel like there's a lot of bikeshed potential if you move into the directions of servers and many pages. I would probably go with a single page and many components. Or you can use existing infra like Create React App. |
I'm not saying do many individual pages, the current strategy of toggling between the different categories of components with a dropdown is a good approach. I'm just saying we can serve this file from
I like that idea! That actually seems like the most logical way to do it. |
Maybe we could even.... get incremental react builds! |
I'm fine with either As for writing the tests themselves: What I like about @aweary's decimal dropping example is that it includes specific reproduction steps for a confusing issue. Does it make sense to have a few pages for those strange edge cases, however maintain large pages by component type like we have now? I can see a few focused examples from my past experience:
So pages for a particular problem. Ones that require special handling inside of React, or we don't want to lose history on. Does this make sense? |
Also, I'm fine punting on specific test cases in favor of the "kitchen sink" style pages until we ship this. To me, they are more of documentation than anything else. @aweary What does your availability look like? Would you like to own setting up a |
@nhunzaker I can own getting a fixtures app setup using |
I think we should have large pages for each category of components like now, and just have the general "kitchen sink" playground at the top, and then have a section below that lists each test case. That way if we provide a checkbox for each one they can just run through the list quickly and verify everything works with their changes, and it's easy to just scroll the page and see if there are any cases you didn't hit |
So I looked into getting an app setup using |
You can just read them from the global. CRA doesn't force you to import React in a specific way.
|
I somehow didn't even think of doing that, thanks! |
c2f86eb
to
99bfa28
Compare
@nhunzaker @gaearon I've pushed my commit with the application created using |
Done. I've also updated the readme with setup instructions. |
f51bc39
to
c79b3f1
Compare
As for inlining the test case descriptions. Every description other than text inputs was pretty low value. Just a quick line that said something like "This fixture tests <textarea />". However the input README is pretty involved, and talks through some important concepts. @gaearon did you envision that we would put that information within the test case? Or rather that we'd place instructions for how to use the test fixture along side the test case? Where should we put something like |
@@ -19,6 +19,8 @@ docs/downloads/*.zip | |||
docs/vendor/bundle | |||
examples/shared/*.js | |||
examples/**/bundle.js | |||
fixtures/dom/public/react-dom.js | |||
fixtures/dom/public/react.js | |||
test/the-files-to-test.generated.js | |||
*.log* | |||
chrome-user-data |
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.
Thanks, 👍
@nhunzaker I think we could have the READMEs explain high-level concepts and implementation details that are good to know, but not crucial for testing. Then we inline instructions that are just to-the-point directions on how to validate expected behavior. That way we don't include too much detail on the fixtures page, but its still available somewhere. |
@aweary that would be my preference as well. |
Sounds good. Really, the only thing necessary is that it's easy both for the team and the casual contributors to understand what they should test and how it should behave. |
👋 just finding this PR. I'd be happy to help out if there is still work to be done :) |
There are a couple of important concepts to be aware of when working on text | ||
inputs in React. | ||
|
||
## `defaultValue` vs `value` |
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.
perhaps worth noting that React explicitly supports changing the value of an input imperatively via JavaScript. This isn't an obvious invariant when making changing to input code, but required to properly support autofilling, password managers, and general DOM interop (unfortunately).
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.
Good catch. We’ll want to add a fixture for this.
// https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input | ||
let types = [ | ||
'text', 'email', 'number', 'url', 'tel', | ||
'color', 'date', 'datetime-local', |
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.
AFAIK datetime-local
is still supported as datetime
in some browsers, I'm not sure if that's worth adding or noting tho.
👋 @jquense we still need to write fixtures for all the specific test cases. Right now we just have the "kitchen sink" pages with controlled/uncontrolled variants of a few input types. I mentioned in #8589 (comment) that we might want to write a component that makes it easy to create new fixtures that can be marked as completed (for users validating behavior when making changes) Any input (heh) you have on scenarios we should be explicitly testing would be amazing! I'll start getting a list together soon and add it to the top comment in this PR. |
Actually I think |
After fixing #8589 (comment) feel free to merge this when you feel like it. |
In addition to fixing #8589 (comment) you'll also need to fix the build fixtures to resolve paths correctly since their relative paths to the build folder are now wrong. You can try to go through the README to see the problem. |
@gaearon: Sorry for going quiet. Busy day! I should be able to get to this feedback tonight. |
@nhunzaker I'm working on it right now, I'll get it fixed and then merged 😃 |
Thanks @aweary! |
@aweary True friend ❤️ |
Webpack requires absolute paths here
module.exports = { | ||
entry: './input', | ||
output: { | ||
filename: 'output.js', | ||
}, | ||
resolve: { | ||
root: '../../build/packages', | ||
root: path.resolve('../../../build/packages/'), |
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.
@gaearon looks like webpack requires resolve.root
to be absolute, had to make this change for the webpack packaging fixtures to build
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.
Got it, thanks
Made the required changes, just waiting on CI to approve and will merge after 👍 |
Related to #8583
This is just the start. I'd like to dynamically pull in versions some how, and @gaearon mentioned renaming
examples
tofixtures
, but it accomplishes most of what we need (I think).Next step would be to create a basic template for a fixture.
@aweary Should we start working from a branch other than master? I can't do that, but I'd be happy to adjust the base of this PR to a
browser-quirks
(or whatever) branch on the main repo.