-
-
Notifications
You must be signed in to change notification settings - Fork 548
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
base: master
Are you sure you want to change the base?
Changes from 9 commits
ae46832
b74e582
066c607
e9d9dcc
967aae2
20a2a7d
1a70274
5f3c3c0
ee6cc50
9c3a1fb
ed2910b
462d202
8c122a2
a225474
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ var MODE_0666 = parseInt('0666', 8) | |
var MODE_0755 = parseInt('0755', 8) | ||
var TEMPLATE_DIR = path.join(__dirname, '..', 'templates') | ||
var VERSION = require('../package').version | ||
var MIN_ES6_VERSION = 14 | ||
|
||
// parse args | ||
var unknown = [] | ||
|
@@ -26,7 +27,7 @@ var args = parseArgs(process.argv.slice(2), { | |
H: 'hogan', | ||
v: 'view' | ||
}, | ||
boolean: ['ejs', 'force', 'git', 'hbs', 'help', 'hogan', 'pug', 'version'], | ||
boolean: ['ejs', 'es6', 'force', 'git', 'hbs', 'help', 'hogan', 'pug', 'version'], | ||
default: { css: true, view: true }, | ||
string: ['css', 'view'], | ||
unknown: function (s) { | ||
|
@@ -93,9 +94,10 @@ function createApplication (name, dir, options, done) { | |
var pkg = { | ||
name: name, | ||
version: '0.0.0', | ||
type: options.es6 ? 'module' : 'commonjs', | ||
private: true, | ||
scripts: { | ||
start: 'node ./bin/www' | ||
start: options.es6 ? 'node ./bin/www.js' : 'node ./bin/www' | ||
}, | ||
dependencies: { | ||
debug: '~2.6.9', | ||
|
@@ -110,6 +112,10 @@ function createApplication (name, dir, options, done) { | |
// App name | ||
www.locals.name = name | ||
|
||
// App module type | ||
app.locals.es6 = options.es6 | ||
www.locals.es6 = options.es6 | ||
|
||
// App modules | ||
app.locals.localModules = Object.create(null) | ||
app.locals.modules = Object.create(null) | ||
|
@@ -160,7 +166,9 @@ function createApplication (name, dir, options, done) { | |
|
||
// copy route templates | ||
mkdir(dir, 'routes') | ||
copyTemplateMulti('js/routes', dir + '/routes', '*.js') | ||
copyTemplateMulti( | ||
options.es6 ? 'mjs/routes' : 'js/routes', | ||
dir + '/routes', '*.js') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having completely separate files/templates for the ES6 style is easier for me to understand quickly. What are you thoughts about using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually, I had it that way on an earlier commit. But I thought that having to maintain parallel files might be a problem, so I went with the merged version. If you prefer parallel files, I'll change back. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed that the benefit for having separate files or even a separate folder of files only for the es6 templates would make the code easier to understand. "easier" here being "less conditionals to read in an ejs file without the benefit for code formatting helping the eyes follow the code". What were your observations related to code readability when you had them separate? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say that it was definitely more readable. The only issue would be, anyone maintaining this would have to remember to make any changes to app.js.ejs in two places, and there would be greater chance of introducing errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the tradeoff would be improving code readability vs reducing Express app template maintenance effort. Both of these are for the future developer (which could be any of us ;) I'm guessing there would have to be an API change in Express to cause a need to change the templates. That possibility might be pretty high given that development activity on Express is going up right now. On the other hand, this feature (--es6) is essentially a different way to write an Express app. Other than the module loading syntax, the differences between the 2 styles can be opaque, but are there none the less. I would argue that having 2 different sets of templates increases the visibility of the design, which in theory, could reduce maintenance effort. Ultimately, it's your call. I just wanted to put my 2 cents out there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My latest push changes to parallel versions of app and www. |
||
|
||
if (options.view) { | ||
// Copy view templates | ||
|
@@ -286,7 +294,7 @@ function createApplication (name, dir, options, done) { | |
write(path.join(dir, 'app.js'), app.render()) | ||
write(path.join(dir, 'package.json'), JSON.stringify(pkg, null, 2) + '\n') | ||
mkdir(dir, 'bin') | ||
write(path.join(dir, 'bin/www'), www.render(), MODE_0755) | ||
write(path.join(dir, options.es6 ? 'bin/www.js' : 'bin/www'), www.render(), MODE_0755) | ||
|
||
var prompt = launchedFromCmd() ? '>' : '$' | ||
|
||
|
@@ -433,6 +441,10 @@ function main (options, done) { | |
usage() | ||
error('option `-v, --view <engine>\' argument missing') | ||
done(1) | ||
} else if (options.es6 && process.versions.node.split('.')[0] < MIN_ES6_VERSION) { | ||
usage() | ||
error('option `--es6\' requires Node version ' + MIN_ES6_VERSION + '.x or higher') | ||
done(1) | ||
} else { | ||
// Path | ||
var destinationPath = options._[0] || '.' | ||
|
@@ -521,6 +533,7 @@ function usage () { | |
console.log(' --no-view use static html instead of view engine') | ||
console.log(' -c, --css <engine> add stylesheet <engine> support (less|stylus|compass|sass) (defaults to plain css)') | ||
console.log(' --git add .gitignore') | ||
console.log(' --es6 generate ES6 code and module-type project (requires Node 14.x or higher)') | ||
console.log(' -f, --force force on non-empty directory') | ||
console.log(' --version output the version number') | ||
console.log(' -h, --help output usage information') | ||
|
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; |
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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ var BIN_PATH = path.resolve(path.dirname(PKG_PATH), require(PKG_PATH).bin.expres | |
var NPM_INSTALL_TIMEOUT = 300000 // 5 minutes | ||
var STDERR_MAX_BUFFER = 5 * 1024 * 1024 // 5mb | ||
var TEMP_DIR = utils.tmpDir() | ||
var MIN_ES6_VERSION = 14 | ||
|
||
describe('express(1)', function () { | ||
after(function (done) { | ||
|
@@ -66,6 +67,7 @@ describe('express(1)', function () { | |
assert.strictEqual(contents, '{\n' + | ||
' "name": "express-1-no-args",\n' + | ||
' "version": "0.0.0",\n' + | ||
' "type": "commonjs",\n' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this still required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I quoted earlier, it's recommended in the Node docs to explicitly set type for every package. So, no, it's not required, but I agree with the thinking that it should be included. |
||
' "private": true,\n' + | ||
' "scripts": {\n' + | ||
' "start": "node ./bin/www"\n' + | ||
|
@@ -484,6 +486,143 @@ describe('express(1)', function () { | |
}) | ||
}) | ||
|
||
describe('--es6', function () { | ||
var ctx = setupTestEnvironment(this.fullTitle()) | ||
|
||
if (process.versions.node.split('.')[0] < MIN_ES6_VERSION) { | ||
it('should exit with code 1', function (done) { | ||
runRaw(ctx.dir, ['--es6'], function (err, code, stdout, stderr) { | ||
if (err) return done(err) | ||
assert.strictEqual(code, 1) | ||
done() | ||
}) | ||
}) | ||
|
||
it('should print usage and error message', function (done) { | ||
runRaw(ctx.dir, ['--es6'], function (err, code, stdout, stderr) { | ||
if (err) return done(err) | ||
assert.ok(/Usage: express /.test(stdout)) | ||
assert.ok(/--help/.test(stdout)) | ||
assert.ok(/--version/.test(stdout)) | ||
var reg = RegExp('error: option `--es6\' requires Node version ' + MIN_ES6_VERSION) | ||
assert.ok(reg.test(stderr)) | ||
done() | ||
}) | ||
}) | ||
} else { | ||
it('should create basic app', function (done) { | ||
run(ctx.dir, ['--es6'], function (err, stdout, warnings) { | ||
if (err) return done(err) | ||
ctx.files = utils.parseCreatedFiles(stdout, ctx.dir) | ||
ctx.stdout = stdout | ||
ctx.warnings = warnings | ||
assert.strictEqual(ctx.files.length, 16) | ||
done() | ||
}) | ||
}) | ||
|
||
it('should print jade view warning', function () { | ||
assert.ok(ctx.warnings.some(function (warn) { | ||
return warn === 'the default view engine will not be jade in future releases\nuse `--view=jade\' or `--help\' for additional options' | ||
})) | ||
}) | ||
|
||
it('should provide debug instructions', function () { | ||
assert.ok(/DEBUG=express-1---es6:\* (?:& )?npm start/.test(ctx.stdout)) | ||
}) | ||
|
||
it('should have basic files', function () { | ||
assert.notStrictEqual(ctx.files.indexOf('bin/www.js'), -1) | ||
assert.notStrictEqual(ctx.files.indexOf('app.js'), -1) | ||
assert.notStrictEqual(ctx.files.indexOf('package.json'), -1) | ||
}) | ||
|
||
it('should have jade templates', function () { | ||
assert.notStrictEqual(ctx.files.indexOf('views/error.jade'), -1) | ||
assert.notStrictEqual(ctx.files.indexOf('views/index.jade'), -1) | ||
assert.notStrictEqual(ctx.files.indexOf('views/layout.jade'), -1) | ||
}) | ||
|
||
it('should have a package.json file with type "module"', function () { | ||
var file = path.resolve(ctx.dir, 'package.json') | ||
var contents = fs.readFileSync(file, 'utf8') | ||
assert.strictEqual(contents, '{\n' + | ||
' "name": "express-1---es6",\n' + | ||
' "version": "0.0.0",\n' + | ||
' "type": "module",\n' + | ||
' "private": true,\n' + | ||
' "scripts": {\n' + | ||
' "start": "node ./bin/www.js"\n' + | ||
' },\n' + | ||
' "dependencies": {\n' + | ||
' "cookie-parser": "~1.4.5",\n' + | ||
' "debug": "~2.6.9",\n' + | ||
' "express": "~4.17.1",\n' + | ||
' "http-errors": "~1.7.2",\n' + | ||
' "jade": "~1.11.0",\n' + | ||
' "morgan": "~1.10.0"\n' + | ||
' }\n' + | ||
'}\n') | ||
}) | ||
|
||
it('should have installable dependencies', function (done) { | ||
this.timeout(NPM_INSTALL_TIMEOUT) | ||
npmInstall(ctx.dir, done) | ||
}) | ||
|
||
it('should export an express app from app.js', function (done) { | ||
// Use eval since otherwise early Nodes choke on import reserved word | ||
// eslint-disable-next-line no-eval | ||
eval( | ||
'const { pathToFileURL } = require("node:url");' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using require("url") fixes the failing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused. When the tests were previously available here in the PR, this test was not failing. On which version is it failing now? Edit: Oh, v15, I see now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, Node v15. I've been focusing on getting the tests to pass for Node 15 just because that's the one that's failing the pipeline, since this change is for node 14 and above. I don't know why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing that 14 works because it was LTS and 15 does not work because it was not LTS. I ran into the same thing between LTS v12, which worked, and non-LTS v13, which did not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for finding this issue! It's fixed in my latest push. |
||
'const file = path.resolve(ctx.dir, "app.js");' + | ||
'import(pathToFileURL(file).href)' + | ||
'.then(moduleNamespaceObject => {' + | ||
'const app = moduleNamespaceObject.default;' + | ||
'assert.strictEqual(typeof app, "function");' + | ||
'assert.strictEqual(typeof app.handle, "function");' + | ||
'done();' + | ||
'})' + | ||
'.catch(reason => done(reason))' | ||
) | ||
}) | ||
|
||
describe('npm start', function () { | ||
before('start app', function () { | ||
this.app = new AppRunner(ctx.dir) | ||
}) | ||
|
||
after('stop app', function (done) { | ||
this.timeout(APP_START_STOP_TIMEOUT) | ||
this.app.stop(done) | ||
}) | ||
|
||
it('should start app', function (done) { | ||
this.timeout(APP_START_STOP_TIMEOUT) | ||
this.app.start(done) | ||
}) | ||
|
||
it('should respond to HTTP request', function (done) { | ||
request(this.app) | ||
.get('/') | ||
.expect(200, /<title>Express<\/title>/, done) | ||
}) | ||
|
||
it('should respond to /users HTTP request', function (done) { | ||
request(this.app) | ||
.get('/users') | ||
.expect(200, /respond with a resource/, done) | ||
}) | ||
|
||
it('should generate a 404', function (done) { | ||
request(this.app) | ||
.get('/does_not_exist') | ||
.expect(404, /<h1>Not Found<\/h1>/, done) | ||
}) | ||
}) | ||
} | ||
}) | ||
|
||
describe('--git', function () { | ||
var ctx = setupTestEnvironment(this.fullTitle()) | ||
|
||
|
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.