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

Add ES module option. #316

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

drjeffjackson
Copy link

@drjeffjackson drjeffjackson commented Apr 4, 2024

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:
image

Eliminate whitespace differences in the generated app.js.
Adding testing. Necessitated adding .js extension to generated bin/www filename.
Remove commented-out code.
@drjeffjackson
Copy link
Author

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.
@drjeffjackson
Copy link
Author

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 const replacing var and arrow functions replacing function expressions in callbacks.

@drjeffjackson
Copy link
Author

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?

@UlisesGascon
Copy link
Member

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.

@ljharb
Copy link

ljharb commented Apr 6, 2024

type module and the node: prefix are both not required, and type module specifically should be avoided. ESM should be .mjs.

@drjeffjackson
Copy link
Author

drjeffjackson commented Apr 6, 2024

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.

@ljharb
Copy link

ljharb commented Apr 6, 2024

The node: prefix works in CJS also, and is not required in either; I'm not sure why it would be useful to conflate that with "use ESM"; similarly, type module is harmful since it violates what .js means, so express should be actively pushing people to use .mjs for ESM, since it's the standard extension for that format.

@joeyguerra
Copy link

@drjeffjackson do you have any plans to work on making the tests in the pipeline to pass?

@drjeffjackson
Copy link
Author

@ljharb Thanks for the additional detail; that's helpful. Regarding your observation:

The node: prefix works in CJS also, and is not required in either; I'm not sure why it would be useful to conflate that with "use ESM"

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:

similarly, type module is harmful since it violates what .js means, so express should be actively pushing people to use .mjs for ESM, since it's the standard extension for that format.

The Node v21 Modules: Packages API says:

This flag [ --experimental-default-type ] currently defaults to "commonjs", but it may change in the future to default to "module". For this reason it is best to be explicit wherever possible; in particular, package authors should always include the "type" field in their package.json files, even in packages where all sources are CommonJS. Being explicit about the type of the package will future-proof the package in case the default type of Node.js ever changes, and it will also make things easier for build tools and loaders to determine how the files in the package should be interpreted.

There is no hint here or anywhere else in the Node docs, as far as I can tell, that type: 'module' is something to avoid. To the contrary, if there is a hint of anything it is that there is a real possibility that the default type of Node will change to module at some point in the future! There is also a relevant implicit assumption here that many of the files in a package, whether a package of type module or type commonjs, will have the .js extension: "Being explicit about the type of the package will ... make things easier ... to determine how the files in the package should be interpreted." If all of the files in a module type package had the .mjs extension, the extension by itself would determine how to interpret the files and the type field would be superfluous. So, the assumption of the quote is that the files matching the package type will likely have .js extensions and that it's therefore good practice to specify the package type, whatever it is.

In addition, a little farther down in the Modules: Packages page we find this:

The .mjs and .cjs extensions can be used to mix types within the same package:

  • Within a "type": "module" package, Node.js can be instructed to interpret a particular file as CommonJS by naming it with a .cjs extension (since both .js and .mjs files are treated as ES modules within a "module" package).
  • Within a "type": "commonjs" package, Node.js can be instructed to interpret a particular file as an ES module by naming it with an .mjs extension (since both .js and .cjs files are treated as CommonJS within a "commonjs" package).

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.
@drjeffjackson
Copy link
Author

@drjeffjackson do you have any plans to work on making the tests in the pipeline to pass?

@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.

copyTemplateMulti('js/routes', dir + '/routes', '*.js')
copyTemplateMulti(
options.es6 ? 'mjs/routes' : 'js/routes',
dir + '/routes', '*.js')
Copy link

@joeyguerra joeyguerra Apr 8, 2024

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?

Copy link
Author

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.

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.

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?

Copy link
Author

@drjeffjackson drjeffjackson Apr 8, 2024

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.

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.

Copy link
Author

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.

@drjeffjackson
Copy link
Author

@drjeffjackson I pulled down the PR to investigate and found a bug in the app.js.ejs template. createError doesn't exist in the --esm -> "should generate a 404" test case. createError is only defined when --esm is not used. Adding the following causes the test to pass:

<% if (esm) { -%>
import express from 'express';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
<% Object.keys(modules).sort().forEach(function (variable) { -%>
import <%- variable %> from '<%- modules[variable] %>';
<% }); -%>
// The following line fixes the test.
import createError from 'http-errors';

I am not sure what you are looking at, @joeyguerra? Here is what I see on my branch at GitHub:

image

@joeyguerra
Copy link

joeyguerra commented Apr 8, 2024

@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.

@drjeffjackson
Copy link
Author

@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");' +

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.

Copy link
Author

@drjeffjackson drjeffjackson Apr 8, 2024

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.

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.

Copy link
Author

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.

Copy link
Author

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.

@joeyguerra
Copy link

@drjeffjackson I think the app.js.ejs code should "work" as it stands in this PR. I had deleted those lines as I was troubleshooting the missing http-errors module error, but forgot to put it back once I resolved it by installing Python 2.

@ljharb
Copy link

ljharb commented Apr 8, 2024

@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.

@joeyguerra
Copy link

2 different viewpoints to the mjs debate.

mdn

For learning and portability purposes, we decided to keep to .js.

V8 team

for Clartity for the reader and the runtime/tooling, use .mjs.

@joeyguerra
Copy link

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' +

Choose a reason for hiding this comment

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

Is this still required?

Copy link
Author

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.

@@ -93,9 +94,10 @@ function createApplication (name, dir, options, done) {
var pkg = {
name: name,
version: '0.0.0',
type: options.es6 ? 'module' : 'commonjs',

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'
  }

Copy link
Author

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.

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.

Copy link
Author

Choose a reason for hiding this comment

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

✔️ Fair enough.

Copy link

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.

Copy link
Author

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.

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.

Copy link
Author

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 ;-)

Copy link
Author

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.

@drjeffjackson
Copy link
Author

@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.

@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?

@ljharb
Copy link

ljharb commented Apr 8, 2024

JavaScript has two parse goals, Script and Module, and since web browsers don't understand file extensions at all, the only runtime uses of .js before ESM was usable in any engine was the Script goal in browsers, and CJS in node. Additionally, CJS isn't just in "old" node code, it's the vast majority of npm's downloads.

The extension should be telling you what module system it is, and if you want to clearly indicate ESM, you'd use .mjs. You shouldn't have to look at package.json to know, and moving the file elsewhere shouldn't lose this out-of-band information when the proper in-band mechanism exists - the extension.

@drjeffjackson
Copy link
Author

drjeffjackson commented Apr 8, 2024

JavaScript has two parse goals, Script and Module, and since web browsers don't understand file extensions at all, the only runtime uses of .js before ESM was usable in any engine was the Script goal in browsers, and CJS in node. Additionally, CJS isn't just in "old" node code, it's the vast majority of npm's downloads.

The extension should be telling you what module system it is, and if you want to clearly indicate ESM, you'd use .mjs. You shouldn't have to look at package.json to know, and moving the file elsewhere shouldn't lose this out-of-band information when the proper in-band mechanism exists - the extension.

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.
@ljharb
Copy link

ljharb commented Apr 9, 2024

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.

@drjeffjackson
Copy link
Author

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.
@joeyguerra
Copy link

joeyguerra commented Apr 10, 2024

I tested this with the following steps (pulled @drjeffjackson repo and esm branch)

npm install
npm link
express myapp --es6
cd myapp
npm install
DEBUG=myapp:* npm start

Visited http://localhost:3000 in my browser.

  • Page was loaded as expected
  • Looked in myapp and saw ES6 style javascript files and code

everything LGTM. Let me know if there's anything else I should look for.

#welldone

@drjeffjackson
Copy link
Author

I tested this with the following steps (pulled @drjeffjackson repo and esm branch) [...]

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.

@sinibida
Copy link

sinibida commented Jul 8, 2024

Looks really promising. When is it going to merge onto the actual package?

templates/mjs/app.js.ejs Outdated Show resolved Hide resolved
templates/mjs/www.ejs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants