Skip to content

Conversation

@marwa7med
Copy link

@marwa7med marwa7med commented Feb 21, 2018

#853

  • remove the body-parser from workshop 👍

Copy link
Member

@eliasmalik eliasmalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are other steps where the body-parser module is referred to, not just step 10. Please make sure you make appropriate changes to those steps as well.

Thanks!

// parse incoming json
app.use(bodyParser.json());
// parse urlencoded bodies
app.use(bodyParser.urlencoded({ extended: false }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you actually run the code after your changes?

Running this file will result in a ReferenceError because you've removed line 5, where bodyParser is defined, yet still refer to it on line 14.

const express = require('express');
const path = require('path');
const favicon = require('serve-favicon');
// import 'body-parser' module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to import the module. See my comment below to understand why.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oooops ! sorry I will do it 👍

@eliasmalik eliasmalik assigned marwa7med and unassigned eliasmalik Feb 22, 2018
@eliasmalik
Copy link
Member

eliasmalik commented Mar 20, 2018

@Marwabj In my review comment I wrote

There are other steps where the body-parser module is referred to, not just step 10. Please make sure you make appropriate changes to those steps as well.

As mentioned here, step 11, and all steps after it, also use the JSON body parser. We can remove those references just like in step 10.

Once you do so I can merge this PR.

@zurda
Copy link
Contributor

zurda commented May 10, 2019

Is this outdated, or is this still in review cc: @astroash @shiryz @oliverjam @m4v15 ?

@zurda zurda requested a review from astroash May 10, 2019 19:07
@oliverjam
Copy link
Contributor

It's been over a year since the last activity so someone else might need to take over if we want to get it merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants