-
Notifications
You must be signed in to change notification settings - Fork 544
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
Add ES module option. #316
Open
drjeffjackson
wants to merge
14
commits into
expressjs:master
Choose a base branch
from
drjeffjackson:esm
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+322
−7
Open
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
ae46832
Add ES module support.
drjeffjackson b74e582
Incorporate ES modules support in ejs files.
drjeffjackson 066c607
Merge branch 'expressjs:master' into esm
drjeffjackson e9d9dcc
Update app.js.ejs
drjeffjackson 967aae2
Add tests
drjeffjackson 20a2a7d
Update cmd.js
drjeffjackson 1a70274
Exit if --esm switch is not supported
drjeffjackson 5f3c3c0
Test fix, update ESM code
drjeffjackson ee6cc50
esm -> es6
drjeffjackson 9c3a1fb
parallel templates, .mjs, fix "should export" test
drjeffjackson ed2910b
Remove type:commonjs.
drjeffjackson 462d202
Remove node: from generated files
drjeffjackson 8c122a2
Merge branch 'expressjs:master' into esm
drjeffjackson a225474
Group Node imports at top of files.
drjeffjackson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
<% if (view) { -%> | ||
import createError from 'http-errors'; | ||
<% } -%> | ||
import express from 'express'; | ||
drjeffjackson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import path from 'node:path'; | ||
import { fileURLToPath } from 'node:url'; | ||
<% Object.keys(modules).sort().forEach(function (variable) { -%> | ||
import <%- variable %> from '<%- modules[variable] %>'; | ||
<% }); -%> | ||
|
||
<% Object.keys(localModules).sort().forEach(function (variable) { -%> | ||
import <%- variable %> from '<%- localModules[variable] %>.mjs'; | ||
<% }); -%> | ||
|
||
const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
|
||
const app = express(); | ||
|
||
<% if (view) { -%> | ||
// view engine setup | ||
<% if (view.render) { -%> | ||
app.engine('<%- view.engine %>', <%- view.render %>); | ||
<% } -%> | ||
app.set('views', path.join(__dirname, 'views')); | ||
app.set('view engine', '<%- view.engine %>'); | ||
|
||
<% } -%> | ||
<% uses.forEach(function (use) { -%> | ||
app.use(<%- use %>); | ||
<% }); -%> | ||
|
||
<% mounts.forEach(function (mount) { -%> | ||
app.use(<%= mount.path %>, <%- mount.code %>); | ||
<% }); -%> | ||
|
||
<% if (view) { -%> | ||
// catch 404 and forward to error handler | ||
app.use((req, res, next) => { | ||
next(createError(404)); | ||
}); | ||
|
||
// error handler | ||
app.use((err, req, res, next) => { | ||
// set locals, only providing error in development | ||
res.locals.message = err.message; | ||
res.locals.error = req.app.get('env') === 'development' ? err : {}; | ||
|
||
// render the error page | ||
res.status(err.status || 500); | ||
res.render('error'); | ||
}); | ||
|
||
<% } -%> | ||
export default app; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import express from 'express'; | ||
const router = express.Router(); | ||
|
||
/* GET home page. */ | ||
router.get('/', (req, res, next) => { | ||
res.render('index', { title: 'Express' }); | ||
}); | ||
|
||
export default router; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import express from 'express'; | ||
const router = express.Router(); | ||
|
||
/* GET users listing. */ | ||
router.get('/', (req, res, next) => { | ||
res.send('respond with a resource'); | ||
}); | ||
|
||
export default router; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
#!/usr/bin/env node | ||
|
||
/** | ||
* Module dependencies. | ||
*/ | ||
|
||
import app from '../app.mjs'; | ||
import http from 'node:http'; | ||
import debugFunction from 'debug'; | ||
const debug = debugFunction('<%- name %>:server'); | ||
|
||
/** | ||
* Get port from environment and store in Express. | ||
*/ | ||
|
||
const port = normalizePort(process.env.PORT || '3000'); | ||
app.set('port', port); | ||
|
||
/** | ||
* Create HTTP server. | ||
*/ | ||
|
||
const server = http.createServer(app); | ||
|
||
/** | ||
* Listen on provided port, on all network interfaces. | ||
*/ | ||
|
||
server.listen(port); | ||
server.on('error', onError); | ||
server.on('listening', onListening); | ||
|
||
/** | ||
* Normalize a port into a number, string, or false. | ||
*/ | ||
|
||
function normalizePort(val) { | ||
const port = parseInt(val, 10); | ||
|
||
if (isNaN(port)) { | ||
// named pipe | ||
return val; | ||
} | ||
|
||
if (port >= 0) { | ||
// port number | ||
return port; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/** | ||
* Event listener for HTTP server "error" event. | ||
*/ | ||
|
||
function onError(error) { | ||
if (error.syscall !== 'listen') { | ||
throw error; | ||
} | ||
|
||
const bind = typeof port === 'string' | ||
? 'Pipe ' + port | ||
: 'Port ' + port; | ||
|
||
// handle specific listen errors with friendly messages | ||
switch (error.code) { | ||
case 'EACCES': | ||
console.error(bind + ' requires elevated privileges'); | ||
process.exit(1); | ||
break; | ||
case 'EADDRINUSE': | ||
console.error(bind + ' is already in use'); | ||
process.exit(1); | ||
break; | ||
default: | ||
throw error; | ||
} | ||
} | ||
|
||
/** | ||
* Event listener for HTTP server "listening" event. | ||
*/ | ||
|
||
function onListening() { | ||
const addr = server.address(); | ||
const bind = typeof addr === 'string' | ||
? 'pipe ' + addr | ||
: 'port ' + addr.port; | ||
debug('Listening on ' + bind); | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered using a conditional and adding the type property instead of defaulting to
commonjs
? The current code doesn't set a type property and defaulting tocommonjs
has a potential to change behavior for existing code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I considered not setting type: commonjs explicitly. I'm almost positive that this will not change existing code. For one thing, express-generator is generating new code ;-) For another, even if someone dropped existing code underneath the generated code, the behavior of the existing code should not change since commonjs is the default for type. All this line does is make this default explicit and "future-proof" the package against possible later changes to the default. That's a good thing, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point about commonjs being the default module loading style for Node. I’m just going to call this out as a risky change because it’s technically impacting existing functionality and can be written in a way to avoid that risk. Ultimately, only time can tell what the positive move is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️ Fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node isn’t going to be able to change the default; it’d break the world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't be too sure of that. After all, many folks thought that Y2K bugs might literally break the world, but they didn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL. I heard someone say that Y2K bugs didn't break the world because of all the planning and work to prepare for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True enough. But in the case of changing the type default, the work for an individual package is adding one line to package.json if it's not already there. One could even imagine automated fixes for this change as opposed to bringing COBOL programmers out of retirement ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All that said, I've made the change you recommended, @joeyguerra. The type field is now only generated when the --es6 switch is active.