Skip to content
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

[examples] Replace Jade with Pug #3183

Closed
wants to merge 9 commits into from
Closed

[examples] Replace Jade with Pug #3183

wants to merge 9 commits into from

Conversation

notrab
Copy link
Contributor

@notrab notrab commented Jan 27, 2017

This fixes #3181

I've gone ahead and replaced all of the examples to use Pug instead of Jade.

@notrab notrab changed the title Replaced Jade example with Pug [example] Replace Jade with Pug Jan 27, 2017
@notrab notrab changed the title [example] Replace Jade with Pug WIP: [examples] Replace Jade with Pug Jan 27, 2017
@@ -4,4 +4,4 @@ p Click a user below to view their pets.
ul
each user in users
li
a(href='/user/#{user.id}')= user.name
a(href='/user/' + user.id)= user.name
Copy link
Contributor Author

@notrab notrab Jan 27, 2017

Choose a reason for hiding this comment

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

Node 4+ would allow this to be written as

a(href=`/user/${user.id}`)= user.name

@notrab notrab changed the title WIP: [examples] Replace Jade with Pug [examples] Replace Jade with Pug Jan 27, 2017
@notrab
Copy link
Contributor Author

notrab commented Jan 27, 2017

@dougwilson Would you be happy to merge this?

@dougwilson
Copy link
Contributor

Hi @notrab this is awesome, thanks so much for putting this together!

@dougwilson
Copy link
Contributor

I've been looking at pug and trying to figure out their Node.js version support. It seems like they broke support for Node.js 0.10 recently, and added it back, but only because of web browsers, not because they wanted to support Node.js 0.10. Their Travis CI config only tests on Node.js 4 & 6. It does seem like beta9 supports Node.js 0.10, so that's good, but all it's deps use ^ which means that we can turn around and can no longer build Express if this suddenly changes.

@dougwilson dougwilson modified the milestone: 4.15 Jan 29, 2017
@dougwilson dougwilson mentioned this pull request Jan 29, 2017
22 tasks
@notrab
Copy link
Contributor Author

notrab commented Jan 29, 2017

Excellent. I wasn't too sure on the dependency tree and Node support. I really wanted to use template literations and some ES6 syntax but I held back so there were no breaking changes in the examples alone.

I've seen a few issues regarding new up to date examples and I more than happy to provide a few but in cases like this, it might be better to pull out some examples into another repo, so those who wish to use newer versions of Node can use things like fat arrows etc and at the same time not breaking Express support for older Node versions.

Now the tests in Express are made from the examples, that seems like a really big task to rewrite for Express so instead I would suggest to remove duplicate examples into their own repo and then the tests will still cover all of the Express features.

I hope this makes sense..!

@dougwilson
Copy link
Contributor

Hi @notrab I currently have it marked to merge into 4.15 branch, but that was before I realized the Node.js 0.10 support issues I noted above, so I'm not sure we can merge this at this time, rather these current changes are great, but without modification in some way would need to wait until we have a branch ready that drops Node.js 0.10 support to merge this into.

@notrab
Copy link
Contributor Author

notrab commented Feb 26, 2017

I've gone ahead and closed this and will create PR's for the things discussed in #3181

Please see #3222 for the first PR.

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.

jade and transformers are deprecated
2 participants