learn-co-curriculum/react-container-components-lab

react-container-components-lab's Issues

React fragment shorthand is not usable

Even though this lab uses react 16.4.x, React fragments shorthand, available in React 16.2, give an unexpected token error:

Failed to compile.

./src/components/SearchableMovieReviewsContainer.js
Syntax error: Unexpected token (15:7)

  13 |   render() {
  14 |     return (
> 15 |       <>
     |        ^
  16 |         <MovieReviews />
  17 |         <div> Searchable</div>
  18 |       </>

<React.Fragment>...</React.Fragment> works normally...

Bad link for NYT Movie Reviews API documentation

The new link is here:
https://developer.nytimes.com/docs/movie-reviews-api/1/overview

Thank you.

Rate Limiting on NYTimes API even with API Key

Unable to complete this lab because NYTimes API sends me a 429 error code: "Too many requests. You reached your per minute or per day rate limit.". I'm not sure if the NYTimes API has changed and added this rate limiter since this lab was created, but if it has this lab will now be incompletable by most students.

Incomplete testing?

There are no tests that test if the app is actually fetching from the API, all the tests just assume there are reviews in.

Error with ReactDOM.render in Index.js

I keep getting this error I can't get rid of it to see what I'm doing in the browser. I'm not sure if it's a user error, or if there's a way to improve the code in index.js to get it to go away.

render
node_modules/react-dom/cjs/react-dom.development.js:27596
./src/index.js
src/index.js:7
   4 | import LatestMovieReviewsContainer from './components/LatestMovieReviewsContainer';
   5 | import SearchableMovieReviewsContainer from './components/SearchableMovieReviewsContainer';
   6 | 
>  7 | ReactDOM.render(
   8 |   <div className="app">
   9 |     <SearchableMovieReviewsContainer />
  10 |     <LatestMovieReviewsContainer />

Missing tests for the API fetching section

There are no tests for the NYT movie reviews API, despite a section for it in the README and some pre-existing code related to it in LatestMovieReviewsContainer.js. All tests pass without ever touching fetch or the API itself.

Restrictive Tests

The way the tests are written they do not allow for any nesting of presentational components as described in previous lessons. For example, creating an individual MovieReview component, or a Search component for the search form causes test failures due to using shallow in the tests.

I recommend using mount instead of shallow specifically for the last test in MovieReviews_test.js (should render all the reviews), and the second to last test in SearchableMovieReviewsContainer_test.js (should fetch data from the New York Times API on form submission). Note that mount has to be imported (same import statement as shallow) in MovieReviews_test.js for this to work, but it is already imported into SearchableMovieReviewsContainer_test.js and just requires a wrapper redefinition using mount in the second to last test).

Making these changes allows further breakdown of presentational components if students want to practice using that pattern.

Issue with MovieReviews Test

The last test for MovieReviews it("should render all the reviews") is testing using Enzyme shallow - which as far as I understand will not render children components. Since it is isolated it cannot find ".review" class name.

The lab asks to make this a functional component and not a Class with state. The beforeEach in the test says if the component is stateless to use Enzyme shallow and if not use Enzyme mount. As I stated above shallow wont work for MovieReviews. The other tests work because they have state and use mount.

Here is the reference I used to figure this out: https://enzymejs.github.io/enzyme/docs/api/ShallowWrapper/shallow.html

I know the lesson doesn't explicitly ask for a Review component to be made. However, following the prior lesson's usage of separation of concerns by making a Book component and BookList component I want to make sure the test would work if other students followed the prior lesson as well.

My fix for this is to replace line 49 of MovieReviews_test.js, which looks like this:
expect(wrapper.find(".review").length).to.equal(testReviews.length);
To this:
expect(wrapper.find(Review).length).to.equal(testReviews.length);
As well as importing Review into the file:
import Review from "../src/components/Review";

No browser output

Aside from the fact that this lab doesn't actually use the API or fetch calls it refers to, it renders nothing in the browser. It would be nice if the API section was tested in such a way that you actually see data in the browser. As of right now, you just get a bunch of tests to pass, and nothing functions or renders... hardly a strong, realistic example.

whatwg-fetch vs node-fetch

In the lesson it states that whatwg-fetch is a good api library for using fetch. However the testing doesn't work with fetch. When using import fetch from 'whatwg-fetch' and also the alternative import 'whatwg-fetch' , fetch comes up undefined. As to where the import fetch from 'whatwg-fetch' comes up with a different error(something about a default).

With node-fetch, all you have to do is const fetch = require('node-fetch') and your done. You can use fetch in the file.

Documentation for node-fetch: https://www.npmjs.com/package/node-fetch

Documentation why whatwg-fetch doesn't work here: JakeChampion/fetch#329

I wanted to bring this up because I spent over half my day trying to make whatwg-fetch work haha. There may of been a way or something i hadn't been able to do to get it working. I wanted to bring up node-fetch because it's easy to use.

Solution contains error

File: https://github.com/learn-co-curriculum/react-container-components-lab/blob/solution/src/components/SearchableMovieReviewsContainer.js

Issue: missing closing tag for state:

state = {
    searchTerm: '',
    reviews: []

  handleSearchInputChange = event => this.setState({ searchTerm: event.target.value });

  handleSubmit = event => {
    event.preventDefault();

    fetch(BASE_URL.concat(this.state.searchTerm))
      .then(res => res.json())
      .then(res => this.setState({ reviews: res.results }));
}

#Staff

Bad link to request NYT API key

New link is here:
https://developer.nytimes.com/get-started

Thank you.

The data IS getting fetched.

The app is working perfectly. The fetch is going through and coming back successfully. But I'm getting this error message:
"

1. should fetch data from the New York Times API on form submission: Error: An error was thrown inside one of your components, but React doesn't know what it was. This is likely due to browser flakiness. React does its best to preserve the "Pause on exceptions" behavior of the DevTools, which requires some DEV-mode only tricks. It's possible that these don't work in your browser. Try triggering the error in production mode, or switching to a modern browser. If you suspect that this is actually an issue with React, please file an issue.

"
It's the only test I'm unable to pass.

NYT API Update

NYT API documentation and links have changed. Please update readme.

Errors lack clarity for resolution

Canvas Link

https://learning.flatironschool.com/courses/1883/assignments/125688?module_item_id=259638

Concern

I'm not sure what the issue is. TDD errors imply that reviews should update when state is changed in container components. This works fine in the browser...

✓ should have state ✓ should have a state property "reviews" ✓ should have top-level element with class "latest-movie-reviews" ✓ should fetch data from the New York Times API 1) should render reviews after reviews state updated ✓ should be a stateless functional component ✓ should have a top-level component with class "review-list" 2) should render all the reviews ✓ should have state ✓ should have the state properties "reviews" and "searchTerm" ✓ should fetch data from the New York Times API on form submission 3) should render reviews after reviews state updated

9 passing (78ms)
3 failing

1. should render reviews after reviews state updated:

   AssertionError: expected 0 to equal 3

   • expected - actual

   -0
   +3

   at Context. (test/LatestMovieReviewsContainer_test.js:52:47)
   at processImmediate (node:internal/timers:464:21)

2. should render all the reviews:

   AssertionError: expected 0 to equal 3

   • expected - actual

   -0
   +3

   at Context. (test/MovieReviews_test.js:45:47)
   at processImmediate (node:internal/timers:464:21)

3. should render reviews after reviews state updated:

   AssertionError: expected 0 to equal 3

   • expected - actual

   -0
   +3

   at Context. (test/SearchableMovieReviewsContainer_test.js:61:47)
   at processImmediate (node:internal/timers:464:21)

Additional Context

I'm sorry that I can't provide much further context. The application seems to function correctly in my browser (updating reviews when state changes in either container component). However, the errors that I'm receiving indicate that this isn't the case.

Feel free to check my work Forked Repo

Suggested Changes

N/A

Need to use a different polyfill for fetch to run the tests

replace line 4 in the /test/root.js file with:

const { fetch } = require('isomorphic-fetch')

otherwise you will get a ReferenceError: fetch is not defined when running mocha or learn in the terminal

Link to create an API key has changed

New link for requesting an API key is 'https://developer.nytimes.com/accounts/create', changed from 'https://developer.nytimes.com/signup'