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

doc: clarify extensionless esm #30657

Closed
wants to merge 5 commits into from
Closed

doc: clarify extensionless esm #30657

wants to merge 5 commits into from

Conversation

@azz
Copy link
Contributor

azz commented Nov 26, 2019

Fixes #30655

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@yorkie
yorkie approved these changes Nov 26, 2019
Copy link
Member

MylesBorins left a comment

I don't think we should be documenting using a non-standard flag here in the docs... but we could improve the documentation above to make the difference between extensionless file path (path has no extension) and extensionless specifiers (path has extension, specifier doesn't, node resolves extension) clearer

@@ -92,6 +92,9 @@ if the nearest parent `package.json` contains `"type": "module"`.
```js
// my-app.js, part of the same example as above
import './startup.js'; // Loaded as ES module because of package.json
// Extensionless. Requires 'node --es-module-specifier-resolution=node'
import './startup'; // Loaded as ES module because of package.json

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Nov 27, 2019

Member

Extensionless files, as documented above, are different from extensionless specifiers (which you are documenting here). Extensionless files mean the file path itself has no extension e.g. /path/to/file, not the specifier e.g. './file' where path is /path/to/file.js.

This comment has been minimized.

Copy link
@azz

azz Nov 27, 2019

Author Contributor

Ah I see. Yeah it could definitely be clarified.

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Dec 3, 2019

Member

Would you be ok removing this line and instead clarifying above?

This comment has been minimized.

Copy link
@azz

azz Dec 3, 2019

Author Contributor

Done. Had trouble writing it succinctly so let me know if it makes sense.

@azz

This comment has been minimized.

Copy link
Contributor Author

azz commented Nov 27, 2019

I don't think we should be documenting using a non-standard flag here

What do you mean by "non-standard flag"?

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Nov 27, 2019

@azz it is an experimental flag, not an agreed upon official feature. Not having it labelled experimental was actually an oversight.

I'm actually working on making that clearer in #30678

@azz azz changed the title doc: clarify required flag for extensionless esm doc: clarify extensionless esm Dec 3, 2019
Copy link
Member

MylesBorins left a comment

Took a quick stab at some slightly different text, unsure if it is better. Happy to see this land with the suggested changes ignore or landed.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
azz and others added 2 commits Dec 5, 2019
Co-Authored-By: Myles Borins <myles.borins@gmail.com>
Co-Authored-By: Myles Borins <myles.borins@gmail.com>
doc/api/esm.md Outdated Show resolved Hide resolved
Co-Authored-By: Myles Borins <myles.borins@gmail.com>
@juanarbol

This comment has been minimized.

Copy link
Contributor

juanarbol commented Dec 18, 2019

I don't know if is necessary, but in #30699, CJS module will give suggestions when file extension is not provided, maybe it would be nice in this docs.

MylesBorins added a commit that referenced this pull request Dec 18, 2019
Fixes: #30655

PR-URL: #30657
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Dec 18, 2019

Landed in 3e5967b

@juanarbol I think documenting #30699 is a great idea, but should likely happen in that PR (so we don't land the docs before the change is landed)

BridgeAR added a commit that referenced this pull request Jan 3, 2020
Fixes: #30655

PR-URL: #30657
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
MylesBorins added a commit that referenced this pull request Jan 12, 2020
Fixes: #30655

PR-URL: #30657
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.