fix(support): support ESM packages & cjs file type #26

aaarichter · 2021-07-21T14:11:01Z

Problems in current nilo version

nilo requires access to the npm package's package.json. If the package declares the exports section but misses to export its package.json, then nilo will receive the error ERR_PACKAGE_PATH_NOT_EXPORTED
if an npm package or a project is type="module", nilo was not able to load any files due to
- project.loadInterfaceFiles('object-graph') is generally called without specific file type
- nilo wouldn't know what file type to look for (js, mjs, cjs)
cjs file type is not supported

Changes

supports ESM packages (type="module") (with or without exports package section)
supports packages with mjs/js files (with or without exports package section)
supports cjs filetype

aaarichter · 2021-07-21T15:02:58Z

hey @jkrems & @aotarola - how are you ?
Would you mind having a look at this PR?
Much appreciated

dbushong · 2021-07-21T17:50:19Z

.github/workflows/node.js.yml

      env:
        NODE_OPTIONS: ${{ matrix.node-options }}


question: Should this be in the section below, not in the node 10 section?

Out of curiosity, why separate 10 out instead of including it in the matrix below?

For node 10 we can run the tests with and without experimental support flag. For Node 12 onwards, Github runs the latest node builds, which all have native support for ESM. For them the NODE_OPTIONS flags make no sense. That's why I separated the node 10 build to avoid having an additional build for every other node version

dbushong · 2021-07-21T17:59:43Z

The import code is (and has always been) particularly tricksy; if you would be willing to write a few sentences on the details of what changed in lib/project.js (and why and how), it would help a lot with the review (and with future contributions) (either inline in comments or narratively here)

aaarichter

hey @dbushong . I added some more information on the change to the PR description as well as comment to the code.

Thank you for reviewing it 🍰

aaarichter · 2021-07-23T15:36:58Z

lib/project.js

+function directRequire(id, cwd) {
+  if (!path.extname(id)) {
+    id = `${id}.js`;
+  }
+  id = path.join(cwd, !isLocalRef(id) ? 'node_modules' : '', id);
+
+  // eslint-disable-next-line import/no-dynamic-require
+  return require(id);


@dbushong this the fallback code that is able to load files when this.require usage fails e.g for ERR_PACKAGE_PATH_NOT_EXPORTED (in this.import() and this.requireOrNull()

aaarichter · 2021-07-23T15:40:30Z

lib/project.js


    const localFiles = await this.cachedGlob(
-      `{modules,lib}/*/${basename}.{js,mjs}`
+      `{modules,lib}/*/${basename}${!extension ? patterns : ''}`


supports extension when calling project.loadInterfaceFiles('object-graph.js')

aaarichter · 2021-07-23T15:43:05Z

lib/project.js

+          for (const ext of extensions) {
+            res = await this.importOrNull(`${specifier}.${ext}`);
+            if (res !== null) {
+              break;
+            }
+          }


iterates over possible extensions - on the first successful import/require it breaks. so if you have mjs and js source files, it picks mjs over js. for js and cjs it would try loading js first

aaarichter · 2021-07-23T15:45:05Z

lib/project.js

-          );
-          if (moduleNamespace === null) return null;
+    /** @type {({default: *}|{default: string}|null|undefined)[]} */
+    const moduleNamespaces = await Promise.all(


I kind of decoupled the logic of require/import and building the interface file array. makes it easier to debug

aaarichter · 2021-07-23T15:47:53Z

lib/project.js

+      // handle ERR_PACKAGE_PATH_NOT_EXPORTED for modules with "exports" section (Node 13/14+)
+      if (e.code === 'ERR_PACKAGE_PATH_NOT_EXPORTED') {
+        try {
+          return { default: directRequire(id, this.root) };


if exports lacks the file declaration require directly. see https://nodejs.org/dist/latest-v14.x/docs/api/packages.html#packages_main_entry_point_export

aaarichter · 2021-07-23T15:51:17Z

lib/project.js

+          return { default: directRequire(id, this.root) };
+        } catch (err) {
+          // eslint-disable-next-line no-ex-assign
+          e = err;


if direct require fails for example with MODULE_NOT_FOUND or ERR_REQUIRE_ESM I'm re-assigning the error, so later code or importOrNull code will handle the errors.

dbushong

Sorry for the delay. LGTM - make sure to test it against a real project (one with esm, one with commonjs, maybe one with both?)

dbushong · 2021-07-24T16:14:46Z

lib/project.js

+  if (!isModule) {
+    return ['mjs', 'js'];


question: At a glance this seems backwards - why are we looking for mjs files if type isn't module? (and vice-versa) Is it because otherwise they'd be found automatically?

so normally the loadInterfaceFiles() method is usually called without any file extension - eg. await project.loadInterfaceFiles('open-graph'). Till now loading local mjs files (if they existed) used to work due the globbing https://github.com/groupon/nilo/blob/main/lib/project.js#L123-L125. By supporting type="module" and also workaround incomplete exports npm package settings, it became necessary to append the file type in order to be able to load the file. Otherwise you will likely get the ERR_REQUIRE_ESM error.

I set the order of those file types is ESM type first, normal type next.

dbushong · 2021-07-24T16:22:39Z

lib/project.js

+  async _getLocalTargets(basename) {
+    const extension = path.extname(basename);
+    const isModule = this.packageJson.type === 'module';
+    const patterns = `.{${getExtensions(isModule).join()}}`;


nitpick: would prefer being explicit about the join character, to avoid people having to look up the quirk that it defaults to ,

Suggested change

const patterns = `.{${getExtensions(isModule).join()}}`;

const patterns = `.{${getExtensions(isModule).join(',')}}`;

dbushong · 2021-07-30T16:37:37Z

lib/project.js

-            await this.importOrNull(specifier)
-          );
-          if (moduleNamespace === null) return null;
+    /** @type {({default: *}|{default: string}|null|undefined)[]} */


Suggested change

/** @type {({default: *}|{default: string}|null|undefined)[]} */

/** @type {({ default: unknown } | null | undefined)[]} */

nitpick (optional): I think unknown is what you want in this case. It's like any or *, but with the guarantee that you won't try to do anything with the contents.

dbushong · 2021-07-30T16:38:22Z

lib/project.js

  }

  /**
   * @param {string} id
-   * @returns {Promise<unknown>}
+   * @returns {Promise<{default: *}|{default: string}>}


nitpick (optional):

Suggested change

* @returns {Promise<{default: *}|{default: string}>}

* @returns {Promise<{ default: unknown }>}

aotarola · 2021-08-26T15:26:06Z

Tested it and works, thanks!

aaarichter requested review from JimLiu, aotarola and erothman-groupon July 21, 2021 14:11

github-actions bot added the semver-patch label Jul 21, 2021

aaarichter force-pushed the fix-file-support branch 2 times, most recently from 87e9d87 to 4354e57 Compare July 21, 2021 14:33

aaarichter changed the title ~~fix(support): support ESM packages & cjs file type'~~ fix(support): support ESM packages & cjs file type Jul 21, 2021

aaarichter requested a review from jkrems July 21, 2021 14:34

fix(support): support ESM packages & cjs file type

84be503

aaarichter force-pushed the fix-file-support branch from 4354e57 to 84be503 Compare July 21, 2021 14:35

Andreas Richter added 2 commits July 21, 2021 16:49

chore(deps): update packages

4131315

test: disable ESM tests for node 10

f0bd5df

dbushong reviewed Jul 21, 2021

View reviewed changes

aaarichter commented Jul 23, 2021

View reviewed changes

aaarichter requested a review from dbushong July 30, 2021 14:06

dbushong approved these changes Jul 30, 2021

View reviewed changes

fix: address feedback

622293b

aaarichter force-pushed the fix-file-support branch from 0640d28 to 622293b Compare August 3, 2021 18:39

aotarola merged commit f87a114 into main Aug 26, 2021

aaarichter deleted the fix-file-support branch September 19, 2021 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(support): support ESM packages & cjs file type #26

fix(support): support ESM packages & cjs file type #26

aaarichter commented Jul 21, 2021 •

edited

Loading

aaarichter commented Jul 21, 2021

dbushong Jul 21, 2021

aaarichter Jul 23, 2021 •

edited

Loading

dbushong commented Jul 21, 2021 •

edited

Loading

aaarichter left a comment

aaarichter Jul 23, 2021

aaarichter Jul 23, 2021

aaarichter Jul 23, 2021

aaarichter Jul 23, 2021

aaarichter Jul 23, 2021

aaarichter Jul 23, 2021

dbushong left a comment

dbushong Jul 24, 2021

aaarichter Aug 3, 2021

dbushong Jul 24, 2021

dbushong Jul 30, 2021 •

edited

Loading

dbushong Jul 30, 2021 •

edited

Loading

aotarola commented Aug 26, 2021

	const patterns = `.{${getExtensions(isModule).join()}}`;
	const patterns = `.{${getExtensions(isModule).join(',')}}`;

	/** @type {({default: }\|{default: string}\|null\|undefined)[]} /
	/** @type {({ default: unknown } \| null \| undefined)[]} */

	* @returns {Promise<{default: *}\|{default: string}>}
	* @returns {Promise<{ default: unknown }>}

fix(support): support ESM packages & cjs file type #26

fix(support): support ESM packages & cjs file type #26

Conversation

aaarichter commented Jul 21, 2021 • edited Loading

Problems in current nilo version

Changes

aaarichter commented Jul 21, 2021

Choose a reason for hiding this comment

aaarichter Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

dbushong commented Jul 21, 2021 • edited Loading

aaarichter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbushong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbushong Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

dbushong Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

aotarola commented Aug 26, 2021

aaarichter commented Jul 21, 2021 •

edited

Loading

aaarichter Jul 23, 2021 •

edited

Loading

dbushong commented Jul 21, 2021 •

edited

Loading

dbushong Jul 30, 2021 •

edited

Loading

dbushong Jul 30, 2021 •

edited

Loading