Skip to content

Conversation

anguspalmer
Copy link

I've picked up Issue #11 and cleaned up grammar in the Readme, moved the instructions regarding Production use into one section and added some code examples for serving the front-end application from Express.
This is my first PR and I'm excited to be making a contribution to an open source project. Please let me know if you want further changes.

Copy link
Owner

@mathieutu mathieutu left a comment

Choose a reason for hiding this comment

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

Hi!

Thank you so much for your contribution!
I've made some feedbacks on it, could you take them in count?

Thanks!

README.md Outdated
```js
var router = express.Router();
router.use(express.static("./dist"));
app.use("/", router);
Copy link
Owner

Choose a reason for hiding this comment

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

All the idea of this package is to avoid that (also, this will not work with vue-router)! shouldServeApp to true (chich is the default) is sufficient to serve properly the app.

Could you remove this part please?

Copy link
Author

@anguspalmer anguspalmer Jul 31, 2019

Choose a reason for hiding this comment

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

I added this as I found it necessary to serve my front-end code. I have also got vue-router working with it.
In my project, shouldServeApp is set to true but when I run yarn express it tells me no routes available and the server isn't serving anything on *localhost:3000*
I've tried this a few times with a fresh project and only the express plugin added but could not make it work.
I've now looked closer at your server.js file and realise the vue app is only served when in production mode. I'll update the documentation to show that as it tripped me up.

On another subject, I was going to create a PR to allow setting the Express port from an environment variable $PORT. Or is there an existing way to do this?

Copy link
Author

Choose a reason for hiding this comment

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

I have removed the code example

anguspalmer and others added 5 commits July 31, 2019 22:16
Co-Authored-By: Mathieu TUDISCO <[email protected]>
Co-Authored-By: Mathieu TUDISCO <[email protected]>
always good to link back to an open issue

Co-Authored-By: Mathieu TUDISCO <[email protected]>
Co-Authored-By: Mathieu TUDISCO <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants