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.

Sign up for GitHub

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

Jump to bottom

fix: update default view engine from jade to pug #328

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: update default view engine from jade to pug #328

wants to merge 1 commit into from

Conversation

RushilJalal
Copy link

@RushilJalal RushilJalal commented Dec 13, 2024 •
edited
Loading

fixes #327

@RushilJalal
Copy link
Author

@UlisesGascon @IamLizu The checks are failing. I require some assistance on how to fix this...

@UlisesGascon
Copy link
Member

From what I can see:
Screenshot from 2024-12-21 15-26-41

It seems like [email protected] has dropped compatibility with the old versions of Node.js that we support (starting from 0.8):
https://github.com/expressjs/generator/actions/runs/12314415858/job/34729590041?pr=328

pug now only supports Node.js@9 and above (see release notes). Ideally, we can include this change in the next major release or upgrade to a version of pug that remains compatible with the Node.js versions we currently support.

@RushilJalal
Copy link
Author

So what is the next step? Do I need to make any changes?

IamLizu
IamLizu suggested changes Dec 30, 2024
View reviewed changes
Copy link
Member

@IamLizu IamLizu left a comment

Choose a reason for hiding this comment

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

we can include this change in the next major release or upgrade to a version of pug that remains compatible with the Node.js versions we currently support.

I think you should do the latter.

bin/express-cli.js
@@ -467,7 +467,7 @@ function main (options, done) {
if (options.view === true) {
warning('the default view engine will not be jade in future releases\n' +
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary warning if jage is no longer the default.

Copy link
Author

Choose a reason for hiding this comment

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

Should I remove this warning completely?

test/cmd.js
' "jade": "~1.11.0",\n' +
// replace default jade with pug
// ' "jade": "~1.11.0",\n' +
' "pug": "~3.0.3",\n' +
Copy link
Member

Choose a reason for hiding this comment

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

Incompatible version of pug

Copy link
Author

Choose a reason for hiding this comment

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

Why is the latest version of pug incompatible? Which version should it be instead?

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

@IamLizu IamLizu IamLizu requested changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

express-generator app uses modules which have critical security vulnerabilities
3 participants
@RushilJalal @UlisesGascon @IamLizu