-
-
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?
Conversation
Add --esm switch that generates a project using ES modules by default.
Eliminate whitespace differences in the generated app.js.
Remove commented-out code.
I'm not sure why 13.x fails when 12.x succeeds, but I'm guessing that it has something to do with 13.x not having LTS. Anyway, I'm pretty sure that the 15.x failure is not related to anything in this PR, because I see the same Python "missing parentheses" error when I run tests on the master branch. For what it's worth, I think that the problem is that some of the Python code being run as part of the test is written in Python 2 syntax that is incompatible with Python 3 while the tests (at GitHub and on my machine) are being run using a Python 3 interpreter. |
Modify the --esm switch processing to error out of the generator if the Node version is less than 14. Update documentation and testing to reflect this change.
Fix esm 'should export an express app from app.js; test failures for early Node versions by moving import into a call to eval(). Also correct two errors in this test and include error handling. In addition, update all generated code when --esm is active to use const rather than var and arrow syntax rather than function expression syntax for callbacks.
The documentation has been updated to state that Node 14.x or higher is required for the --esm switch, and the --esm switch should now lead to an error exit for all Node versions prior to 14, The only tests run for these earlier versions are to check for an error exit and for appropriate error messages. I believe that the esm automated tests will now pass for all Node versions, with versions 14 and higher running the 12 tests shown in the earlier image. However, testing with Node 15.x and higher still produces errors on my machine related to sass, but this is true on the master branch as well. The files generated in --esm mode now use updated JavaScript syntax, with |
Am I correct in thinking that all of the PR test failures also occur when testing the master branch, or do I need to do some more work on the PR related to automated tests? |
I think that there are some PRs with solutions for the CI, let me check. Probably we should merge them first. |
type module and the node: prefix are both not required, and type module specifically should be avoided. ESM should be .mjs. |
Agreed, neither is required. The point of the PR is to provide an option to those who would prefer to begin a new project as ESM and to use other newer Node/JavaScript features such as the node: prefix and ES6 syntax. The Node API docs going back to v16 show example code in both CJS and ESM syntax. It seems past time for express-generator to provide the same option. |
The |
@drjeffjackson do you have any plans to work on making the tests in the pipeline to pass? |
@ljharb Thanks for the additional detail; that's helpful. Regarding your observation:
You're absolutely right. My original goal was to generate an ESM version of the skeleton code, but in implementing this I realized that this felt like a halfway measure and that what would be even better would be to generate an ES6 version of the code. Despite my development going in that direction, I still referred to this as an ESM mod. That was an oversight, and I appreciate your pointing this out. I'll be pushing a commit renaming the switch to --es6. However, regarding your claim that:
The Node v21 Modules: Packages API says:
There is no hint here or anywhere else in the Node docs, as far as I can tell, that In addition, a little farther down in the Modules: Packages page we find this:
Again, there's no hint here that anything is violated by using a "type": "module" package containing .js files that contain ESM code, any more than there is anything wrong with using a "type": "commonjs" package containing .js files that contain CJS code. In short, I've looked, but I haven't found any support for your claim while I have found what seem to me to be strong indicators to the contrary. However, maybe I've missed something. I'd be interested in any pointers you have to reputable recent recommendations that "type": "module" be avoided in new projects. |
Change switch from esm to es6 to better reflect what the switch does.
@joeyguerra: I believe that my PR code passes all of the tests that I added for it and causes no problems with existing tests. That is, I believe that all of the test failures are due to other code that, as far as I can tell, also fails on the master branch. @UlisesGascon is I believe looking into fixing these tests, so no, I have no plans currently to correct these problems since they're not problems with my code/tests. That said, if it is a problem for others to fix the failing tests and if all tests need to pass before my PR can be approved, it would not be hard for me to submit a separate PR that skips the failing tests on a version-specific basis, such as adding code that for v0.10 skips the pug tests that fail. |
bin/express-cli.js
Outdated
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 comment
The 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 .mjs
file extension and creating a different .mjs
template? That would remove the need for conditional logic in the app.js.ejs
file. If not the mjs
file extension, something else in the filename or folder to distinguish it from the Common JS style?
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.
Having completely separate files/templates for the ES6 style is easier for me to understand quickly. What are you thoughts about using
.mjs
file extension and creating a different.mjs
template? That would remove the need for conditional logic in theapp.js.ejs
file.
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 comment
The 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 comment
The 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.
Also, what about www? I had that in two places originally.
As for the approach, I had added app.js.ejs and www.js.ejs to the mjs folder that this PR adds to the file structure.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
My latest push changes to parallel versions of app and www.
I am not sure what you are looking at, @joeyguerra? Here is what I see on my branch at GitHub: |
@drjeffjackson you're correct. I had removed lines 2 through 4 when troubleshooting the "http-errors not found" issue in the node 15 pipeline run. |
Ah, okay. So the issue is in the master-branch code, which imports createError conditioned on view. My code does the same thing, and I think that you're saying that createError needs to be imported unconditionally in both versions. Anyway, sure, I can make that change in my code. Would you want me to change the original code to match as well? |
test/cmd.js
Outdated
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
using require("url") fixes the failing should export an express app from app.js
test case.
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'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 comment
The 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 eval
can't find node:url
. Would love to know.
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'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 comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this issue! It's fixed in my latest push.
@drjeffjackson I think the |
@drjeffjackson the docs are written by folks with an agenda. I was on the modules working group, and i maintain module tooling, and I’m on the language committee, and i can assure you that .mjs is the standard extension for ESM; changing the default of the “type” field won’t affect anyone who uses .mjs - only those who use .js. It’s not that using type module “violates” anything, it’s that it’s sole purpose is to subvert why “.js” means - from CJS to ESM - and that can be bypassed simply by using the standard ESM file extension. |
Btw, PR #314 adds Python 2 to the pipeline runner which I think would make all the pipeline tests in this PR pass. |
test/cmd.js
Outdated
@@ -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 comment
The 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 comment
The 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.
bin/express-cli.js
Outdated
@@ -93,9 +94,10 @@ function createApplication (name, dir, options, done) { | |||
var pkg = { | |||
name: name, | |||
version: '0.0.0', | |||
type: options.es6 ? 'module' : 'commonjs', |
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 to commonjs
has a potential to change behavior for existing code.
var pkg = {
name: name,
version: '0.0.0',
private: true,
scripts: {
start: options.esm ? 'node ./bin/www.js' : 'node ./bin/www'
},
dependencies: {
debug: '~2.6.9',
express: '~4.17.1'
}
}
if (options.esm) {
pkg.type = 'module'
}
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.
@ljharb Impressive resume, but I notice that you don't seem to have anything to point to to back up your claim about the type field other than your own informed opinion. I like to think that I have an impressive resume and informed opinions as well, and in my opinion new code should be written using ES6+, including ES modules. It's simply a better way to code. Also in my opinion, setting type to module is simply a way of encouraging that desirable behavior in new projects. As for subverting the meaning of .js, .js means that a file contains JavaScript, pure and simple. Yes, if you look at old Node code, you are going to expect to see the CJS module system used in many .js files. But the question of interest is, what module system should we expect to see used in new Node .js files? I don't see any problem with saying that we don't know what to expect but that in a well-written package we will be able to look at package.json to find out. How is this subversive? |
JavaScript has two parse goals, Script and Module, and since web browsers don't understand file extensions at all, the only runtime uses of The extension should be telling you what module system it is, and if you want to clearly indicate ESM, you'd use |
I agree that using .mjs has certain advantages. It also has some disadvantages, as noted in the MDN article. If the module type can stay in, I can generate files with .mjs extensions. My latest push implements this change. |
Use parallel template files for non-es6 and es6 versions of app and www. In es6 mode, use .mjs extension on all generated JavaScript files. Remove node: in the es6 "should export" test's require of url.
The type field is now added to package.json only when the generator is run with the --es6 switch.
btw i think "es6" isn't a good name; ES6 is ES2015, which was 9 years ago, and there's lots of module features added since then - and node didn't ship ESM support until 2020. if it's a broad "use one person's idea of modern practices" then a generic name like "modern" is probably more appropriate; if it's named something like "ESM" it should omit subjective opinions. |
@ljharb All of the changes to the generated code involve language features introduced by ES6: JavaScript modules, const, and arrow functions. So, --es6 is appropriate. Yes, the switch also assumes that new code written for the project will use the module syntax introduced by ES6 and sets the package type accordingly. That seems more appropriate to me than assuming otherwise, which would be the implicit assumption were the package type not specified. |
The node: URL scheme was not available in v14 until v14.13.1. So, removing this scheme to stay compatible with all releases of v14.
I tested this with the following steps (pulled @drjeffjackson repo and npm install
npm link
express myapp --es6
cd myapp
npm install
DEBUG=myapp:* npm start Visited http://localhost:3000 in my browser.
everything LGTM. Let me know if there's anything else I should look for. #welldone |
Thanks, @joeyguerra! One other test would be visiting http://localhost:3000/users, which should respond with the text "respond with a resource". And you should also see ES6 style in bin/www, routes/index, and routes/users. |
Looks really promising. When is it going to merge onto the actual package? |
Per recommendation by aagamezl.
Adds an --esm switch with documentation in both the README and the app's usage message. When this switch is active, the bin/www (see below about the name of this file), app.js, and routes files all use ES module import/export syntax instead of Common JS require() syntax. Other small code modifications in support of this change are made in --esm mode, such as adding .js to some filename strings, defining a __dirname variable, and adding 'node:' to Node library references (I believe that this requires the end user to be running Node version 16 or greater, which hopefully isn't a problem; it's easy to back out the change if it is a problem). One change that affects the files generated in non-esm mode is that the generated package.json file now always includes a 'type' field that has value of either 'module' when the --esm switch is active or 'commonjs' by default.
The generator code structure has been modified by adding a template/mjs folder in parallel with the existing template/js folder. The mjs folder contains the ESM versions of the example route files. This means that any future changes to the example route files should be made in two places to maintain compatibility between the CJS and ESM. versions. An alternative to the parallel folder structure would be to generate the route files from ejs templates rather than to follow the existing approach of copying them verbatim. I went with the simpler existing approach under the assumption that the example route files will change very infrequently if at all.
Testing of --esm has been added to test/cmd.js. Some of this testing failed when attempting to load bin/www due to the lack of file extension. So, when --esm is active this file is named bin/www.js. The --esm tests are based on the (no-args) tests but do not repeat the directory-oriented tests: