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

esm: add gotcha to documentation #32237

Closed
wants to merge 2 commits into from
Closed

esm: add gotcha to documentation #32237

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bec61e
Update esm.md
jonerer Mar 12, 2020
ea679b7
esm: Update documentation with windows gotcha
Mar 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions doc/api/esm.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,22 @@ property:

## Differences Between ES Modules and CommonJS

### Absolute file paths don't work on Windows
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a section on module specifiers in the "Terminology" section above. I'd rather we move this note into that section along with the description of module specifiers, than as a specific difference between CJS and ESM.

Strictly speaking, yes it is a difference, but I think to try keep the docs organized we should keep sections together.

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. But this note does need to specifically mention Windows, for Ctrl+F compatibility. My idea was that platform differences belonged in a "gotchas" type section.

But I'm new to writing the node.js docs. Would you rather have it up by the "specifiers" section?

Copy link
Contributor

@guybedford guybedford Mar 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I can get behind having this section be a specific compat issue section.

Can we change the title then to:

Suggested change
### Absolute file paths don't work on Windows
### File URLs not file paths

and move it to after the extensions section, since that should probably still come first.

Then perhaps with the description, we can get straight to the problem rather than having it in the title:

On Windows, importing file paths will throw since the drive letter is interpreted
as a URL protocol. Instead a URL conversion is needed...

or something like that?

Copy link
Member

Choose a reason for hiding this comment

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

if you're importing a path, conversion is always needed!

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 concerned that's a bit backwards; I would prefer to catch Windows' users eyes with the title, to make sure they understand it's an important section. And then go into the technical reasons about why it doesnt work for them in the body.

But again, I'm new here. Is your method more inline with how the docs are typically worded?

Copy link
Author

@jonerer jonerer Mar 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok. My primary concern was to bring the documentation in line with experience for Windows users (and for makers of libraries supporting Windows).

Another way to resolve this issue seems to be to change the implementation of ES Modules in node.js to enforce the specifiers not allowing a starting char of '/'. And to change this documentation PR to one that is general for both MacOS and Windows ("File URLs not file paths").

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

it is valid for a relative url to start with / and i don't see any reason to limit that. In terms of changes to the runtime, adding a more detailed error message in ERR_UNSUPPORTED_ESM_URL_SCHEME when the scheme is a single letter might help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree for the windows-specific bug, adding more context to the error message would be preferable. Then to keep the docs changes here in line with the suggestion above to provide the more general advice since this is not Windows-specific information (eg there are other character encoding issues that apply for all platforms).

Copy link
Author

@jonerer jonerer Mar 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That would be a really good way to highlight the error when it happens. Definitely a good change!

Regarding absolute file paths when running on MacOS. Can we put some code to detect if the user is trying to do absolute path imports, and warn them that this is unsupported? Like comparing the fileToUrl() output with "file://" + specifier to see if it's the same?

Right now there's the issue of library creators using import() in unsupported ways. For libraries that auto-import files, it's likely that they will implement loading on MacOS, find that absolute paths work (by luck) and ship that. Which causes errors for Windows users of those libraries.

This already happened with jest and babel. But I'm sure more lower-profile projects will follow, resulting in undetected non-platform-independent libraries on npm.

I think that there is an expectation that you can produce platform-independent code by sticking with the built in functions (except for the obvious ones like child_process and some fs functions). And I would like to explore what possibilities there are to keep it that way

Copy link
Member

Choose a reason for hiding this comment

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

The last subsection of the “Differences between ES Modules and CommonJS” section is “URL-based paths”. I think that the warning should go in there, either inside that section or in a new section after it.

All the other gotchas are platform-agnostic, and therefore more important than something Windows-specific (since they affect everyone, not just Windows users). I understand that Windows users are a majority of all users, but absolute paths are also not that common of a use case; when combining both a gotcha that doesn’t affect everyone with a feature that isn’t that commonly used (absolute paths), I think it should go near the end of the list.


`import` accepts _absolute specifiers_. These are similar to _absolute paths_
on linux and MacOS, and therefore they are also accepted.
But on Windows, importing an _absolute path_ to a file will not work.
To ensure cross platform compatibility, file paths should be converted into
URLs.
Copy link
Member

Choose a reason for hiding this comment

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

As a matter of style, the Node docs avoid italics/unnecessary emphasis whenever possible.


```js
import { pathToFileURL } from 'url';

const fileUrl = pathToFileURL('c:\\path\\to\\file.js').href;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const fileUrl = pathToFileURL('c:\\path\\to\\file.js').href;
const fileUrl = pathToFileURL('C:\\path\\to\\file.js').href;

uppercase is necessary. We are using C: everywhere else in those docs.

related PR: #32294


await import(fileUrl);
Copy link
Member

Choose a reason for hiding this comment

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

example:

Suggested change
await import(fileUrl);
(async function() {
await import(fileUrl);
})();

Copy link
Author

Choose a reason for hiding this comment

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

I feel the code snipped should be short and to the point. An iife is a bit distracting. Maybe adding a ".then(...)" instead of await?

Copy link
Member

Choose a reason for hiding this comment

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

good

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment above noting what the value of fileUrl is:

// file:///c:/path/to/file.js

or similar, just so it is clear what is happening.

```

### Mandatory file extensions

A file extension must be provided when using the `import` keyword. Directory
Expand Down