esm: doc & validate source values for formats #32202

bmeck · 2020-03-11T15:44:20Z

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

This better specifies how decoding of source values returned from loaders are translated. Some formats expect different types of sources. This also fixes some problems where Buffers were properly decoding due to the custom .toString but typed arrays were not.

We could expand the tests if desired, but I felt just checking Uint8Array was sufficient.

GeoffreyBooth

Looks good other than the docs note. Thanks for doing this!

GeoffreyBooth · 2020-03-11T18:36:30Z

doc/api/esm.md


-| `format` | Description |
+| `format` | Description | Acceptable Source Values |


Maybe it's just me, but I read this as "the source value of format", e.g. that 'builtin' should be a string. I think what you mean is the acceptable type of source returned by getSource for this format? So maybe call this "Acceptable Types for getSource Returned source"?

Should we also update the signature in getSource?

@returns {string|buffer} response.source

Yes, I meant the value from either getSource OR transformSource

Okay, do you mind making that clearer? Maybe “Acceptable Types For source Returned by getSource or transformSource”?

done, also made sure to actually check both hooks rather than just transformSource

guybedford

Nice to see this being locked down well.

GeoffreyBooth · 2020-03-13T22:03:38Z

Do we need to update the JSDoc function signatures? e.g.

@returns {string|buffer} response.source

nodejs-github-bot · 2020-03-20T20:34:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29959/

GeoffreyBooth · 2020-03-20T20:57:08Z

@bmeck ☝️ #32202 (comment) do we need to fix this too?

bmeck · 2020-03-23T14:36:14Z

@GeoffreyBooth We could, but overall the signature isn't trivial and I think Buffer|string is the most likely realistic return value people want to know about. It would be ideal if we had something more sane to explain all the coercions code does in core (like what WebIDL provides), but we don't in core for now :(.

bmeck · 2020-03-23T14:54:50Z

rebased and looks green locally, will CI then land if green and no comment in next few hours

bmeck · 2020-03-23T15:29:59Z

rebase had a minor change, in particular I had to update ./test/fixtures/es-module-loaders/transform-source.mjs to handle if it gets a buffer back for a module format.

nodejs-github-bot · 2020-03-24T16:20:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30022/

addaleax · 2020-03-29T20:07:02Z

@bmeck Didn’t look into the other failures but at least the linter is complaining

nodejs-github-bot · 2020-03-31T16:27:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30285/

bmeck · 2020-03-31T17:10:32Z

Fixed lint, CI errors seem unrelated

nodejs-github-bot · 2020-04-17T16:22:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30813/

nodejs-github-bot · 2020-05-09T13:10:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31256/

targos · 2020-05-09T13:43:35Z

Test error without intl:

assert.js:103
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: These modules were unexpectedly loaded:
  NativeModule string_decoder

    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-bootstrap-modules.js:134:8)
    at Module._compile (internal/modules/cjs/loader.js:1176:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1196:10)
    at Module.load (internal/modules/cjs/loader.js:1040:32)
    at Function.Module._load (internal/modules/cjs/loader.js:929:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: [Set],
  expected: [Set],
  operator: 'deepStrictEqual'
}

doc/api/esm.md

Co-authored-by: Derek Lewis <[email protected]>

doc/api/esm.md

Co-authored-by: Derek Lewis <[email protected]>

nodejs-github-bot · 2020-05-19T00:20:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31398/

doc/api/esm.md

DerekNonGeneric

Although much of this is new material to me, the previous layout didn't really make sense to me. :/

My suggestions are how I've made sense of it, but I could be wrong. I hope we can improve this little area, it's a rather important section for those implementing types on the function boundaries of the custom loader hooks API.

doc/api/esm.md

Co-authored-by: Derek Lewis <[email protected]>

nodejs-github-bot · 2020-05-20T12:33:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31445/

nodejs-github-bot · 2020-05-20T14:42:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31448/

PR-URL: #32202 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

himself65 · 2020-05-21T05:02:57Z

landed in cd4985c

PR-URL: #32202 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Zeyu Yang <[email protected]>

bmeck added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 11, 2020

bmeck requested a review from GeoffreyBooth March 11, 2020 15:44

GeoffreyBooth reviewed Mar 11, 2020

View reviewed changes

guybedford approved these changes Mar 12, 2020

View reviewed changes

GeoffreyBooth approved these changes Mar 13, 2020

View reviewed changes

bmeck force-pushed the specify-esm-source-decoding branch from 44fcb28 to a3a01f4 Compare March 23, 2020 15:29

bmeck requested a review from GeoffreyBooth March 23, 2020 15:30

GeoffreyBooth approved these changes Mar 24, 2020

View reviewed changes

bmeck force-pushed the specify-esm-source-decoding branch from a3a01f4 to cd5ccdf Compare March 31, 2020 16:15

targos approved these changes May 9, 2020

View reviewed changes

guybedford mentioned this pull request May 17, 2020

module: fix initial ModuleWrap with non-string source #33443

Closed

3 tasks

bmeck force-pushed the specify-esm-source-decoding branch from ae2427c to 01af9cd Compare May 18, 2020 18:03

esm: doc & validate source values for formats

614ec8e

bmeck force-pushed the specify-esm-source-decoding branch from 01af9cd to 614ec8e Compare May 18, 2020 18:04

DerekNonGeneric reviewed May 18, 2020

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

Update doc/api/esm.md

d2c1e51

Co-authored-by: Derek Lewis <[email protected]>

DerekNonGeneric reviewed May 18, 2020

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

Update doc/api/esm.md

ecb72bc

Co-authored-by: Derek Lewis <[email protected]>

himself65 approved these changes May 19, 2020

View reviewed changes

DerekNonGeneric reviewed May 19, 2020

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

DerekNonGeneric reviewed May 19, 2020

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

doc/api/esm.md Show resolved Hide resolved

bmeck and others added 3 commits May 19, 2020 14:02

Update doc/api/esm.md

d7d8024

Co-authored-by: Derek Lewis <[email protected]>

Update doc/api/esm.md

759e4f2

Co-authored-by: Derek Lewis <[email protected]>

Update doc/api/esm.md

525554d

Co-authored-by: Derek Lewis <[email protected]>

GeoffreyBooth approved these changes May 19, 2020

View reviewed changes

himself65 closed this May 21, 2020

himself65 mentioned this pull request May 21, 2020

Crash in module_wrap.cc with malformed loader #33441

Closed

GeoffreyBooth mentioned this pull request May 28, 2020

esm,doc: fix custom loader hook snippet typings #33563

Closed

3 tasks

codebytere mentioned this pull request Jun 28, 2020

v14.5.0 proposal #34093

Merged

codebytere mentioned this pull request Jul 13, 2020

v12.18.3 proposal #34343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: doc & validate source values for formats #32202

esm: doc & validate source values for formats #32202

bmeck commented Mar 11, 2020

GeoffreyBooth left a comment

GeoffreyBooth Mar 11, 2020

bmeck Mar 11, 2020

GeoffreyBooth Mar 11, 2020

bmeck Mar 12, 2020

guybedford left a comment

GeoffreyBooth commented Mar 13, 2020

nodejs-github-bot commented Mar 20, 2020

GeoffreyBooth commented Mar 20, 2020

bmeck commented Mar 23, 2020 •

edited

Loading

bmeck commented Mar 23, 2020

bmeck commented Mar 23, 2020

nodejs-github-bot commented Mar 24, 2020

addaleax commented Mar 29, 2020

nodejs-github-bot commented Mar 31, 2020

bmeck commented Mar 31, 2020

nodejs-github-bot commented Apr 17, 2020

nodejs-github-bot commented May 9, 2020

targos commented May 9, 2020

nodejs-github-bot commented May 19, 2020

DerekNonGeneric left a comment •

edited

Loading

nodejs-github-bot commented May 20, 2020

nodejs-github-bot commented May 20, 2020

himself65 commented May 21, 2020


		\| `format` \| Description \|
		\| `format` \| Description \| Acceptable Source Values \|

esm: doc & validate source values for formats #32202

esm: doc & validate source values for formats #32202

Conversation

bmeck commented Mar 11, 2020

Checklist

GeoffreyBooth left a comment

Choose a reason for hiding this comment

GeoffreyBooth Mar 11, 2020

Choose a reason for hiding this comment

bmeck Mar 11, 2020

Choose a reason for hiding this comment

GeoffreyBooth Mar 11, 2020

Choose a reason for hiding this comment

bmeck Mar 12, 2020

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

GeoffreyBooth commented Mar 13, 2020

nodejs-github-bot commented Mar 20, 2020

GeoffreyBooth commented Mar 20, 2020

bmeck commented Mar 23, 2020 • edited Loading

bmeck commented Mar 23, 2020

bmeck commented Mar 23, 2020

nodejs-github-bot commented Mar 24, 2020

addaleax commented Mar 29, 2020

nodejs-github-bot commented Mar 31, 2020

bmeck commented Mar 31, 2020

nodejs-github-bot commented Apr 17, 2020

nodejs-github-bot commented May 9, 2020

targos commented May 9, 2020

nodejs-github-bot commented May 19, 2020

DerekNonGeneric left a comment • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented May 20, 2020

nodejs-github-bot commented May 20, 2020

himself65 commented May 21, 2020

bmeck commented Mar 23, 2020 •

edited

Loading

DerekNonGeneric left a comment •

edited

Loading