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

esm: make enhancements to example loader fixture #32646

Closed
wants to merge 1 commit into from
Closed

esm: make enhancements to example loader fixture #32646

wants to merge 1 commit into from

Conversation

DerekNonGeneric
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric commented Apr 3, 2020

  • Add JSDoc typings to function boundaries
  • Remove import of URL & process as they are unnecessary
  • Update bare-specifier check to match language used in error
  • Improve accuracy of hook's thrown error message
  • Move destructuring assignment of default to within function

Refs: #31303

/cc @nodejs/modules-active-members

Checklist

doc/api/esm.md Outdated Show resolved Hide resolved
* Add JSDoc typings to function boundaries
* Remove import of URL & process as they are unnecessary
* Update bare-specifier check to match language used in error
* Improve accuracy of hook's thrown error message
* Move destructuring assignment of default to within function

Refs: #31303
@devsnek
Copy link
Member

devsnek commented Apr 6, 2020

Fixtures aren't example code for people to follow, they're blobs of code used in our tests. Items in test should definitely not be used for examples of best practice.

@DerekNonGeneric
Copy link
Contributor Author

This strikes me as more of an integration test of the hooks.

@Trott
Copy link
Member

Trott commented Apr 19, 2020

For addons.md, we have code that extracts the code samples and makes sure they all compile. Perhaps we could do something similar for esm.md?

@DerekNonGeneric DerekNonGeneric marked this pull request as draft April 21, 2020 05:34
@DerekNonGeneric
Copy link
Contributor Author

DerekNonGeneric commented Apr 29, 2020

According to the documentation style guide, this work should be located elsewhere.

* Code need not be complete. Treat code blocks as an illustration or aid to
your point, not as complete running programs. If a complete running program
is necessary, include it as an asset in `assets/code-examples` and link to
it.

Unless this test fixture is supposed to serve as some sort of poor-man's integration test of the hooks, I would be inclined to move this example out of this directory and into the location specified by the documentation style guide, however, this directory (assets/code-examples) does not appear to exist.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@targos targos added the esm Issues and PRs related to the ECMAScript Modules implementation. label Dec 27, 2020
@DerekNonGeneric DerekNonGeneric marked this pull request as ready for review May 17, 2022 15:32
@DerekNonGeneric DerekNonGeneric marked this pull request as draft May 17, 2022 15:33
@aduh95
Copy link
Contributor

aduh95 commented Sep 19, 2023

The loader API has changed a lot since this PR was opened, it seems this PR no longer applies. Closing.

@aduh95 aduh95 closed this Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants