fix: add back CommonJS output

[johnhooks]

@aduth Gutenberg built beautifully and works with this module as an ES Module, though with the current Jest config it can't load ES Modules. I'm sorry I should have run the tests before we finished the #10.

[aduth]

I had a feeling that going all-in on ESM might cause some trouble downstream. I think at some point it'd become reasonable to drop formal support for CommonJS, though if Gutenberg still relies on it and it's one of the project's biggest consumers, it probably makes sense to keep supporting for now.

[johnhooks]

@aduth, sad news it doesn't work with your suggestion, main has to point to the CommonJS. What do you think we should do?

Also, there was one test in the Gutenberg suite that relied on getCache() which I still had being replaced in cjs.

[aduth]

@aduth, sad news it doesn't work with your suggestion, main has to point to the CommonJS. What do you think we should do?

Hm, that's strange. I was able to reproduce the original issue with a simple CommonJS project ("Error [ERR_REQUIRE_ESM]: require() of ES Module not supported."), but the error went away with the introduction of exports. I wonder if there's something unique about Gutenberg. Can you share more detail about the issue you're seeing?

Worst case, I think we could invert the default by reverting back to CommonJS with dropping "type": "module" and point to the CommonJS as .js, with exports supporting import as .mjs.

{
  "main": "dist/index.js",
  "exports": {
    "require": "./dist/index.js",
    "import": "./dist/index.mjs"
  }
}

Also, there was one test in the Gutenberg suite that relied on getCache() which I still had being replaced in cjs.

That function was never meant to be part of a public API, so I don't feel too bad about that breaking. Could that function call or test be removed? I don't even like that it's relied on in Memize's own tests 😅 and was contemplating removing it altogether.

[johnhooks]

That function was never meant to be part of a public API, so I don't feel too bad about that breaking. Could that function call or test be removed? I don't even like that it's relied on in Memize's own tests 😅 and was contemplating removing it altogether.

I can look into this.

Can you share more detail about the issue you're seeing

The error I was getting was from Jest, it pointed to either transpiling with babel, or enabling support for es modules in Jest https://jestjs.io/docs/ecmascript-modules, it looks like it might load it appropriately if it has a .mjs extension, I'll try that.

Edit:

@aduth the solution above does work "main": "dist/index.mjs". I find it inelegant, but it should be supported.

I'll test if we can do that, and drop the cjs output. Nope, it seems that Jest will use the .cjs in the exports since the main export is a .mjs file... I guess. Because it can't load the dist/index.mjs file even with the extension, but having the extension prevents the issue.

[aduth]

I was able to confirm the issues you're seeing in Gutenberg when testing on your branch at WordPress/gutenberg#48604 . It's still strange that the exports is not respected, though I suspect that's either related to Gutenberg using an old version of Node.js (v14) or Jest having poor support overall for ESM.

The fact that it works with the .mjs extension is interesting. I think it sorta defeats the purpose of using "type": "module" to define the default treatment of .js files as ESM, but I suppose there's still value in it since we rely on it for Rollup configuration and the specs as well.

I'd be alright with moving forward with that solution if it's working in Gutenberg.

[johnhooks]

It builds and the full test suite passes. I think there is work to switch Gutenberg to a new version of node., it might be jumping to v18. When that happens, we can experiment with dropping the .mjs extension.

[aduth]

Curious how you were testing, because one thing I noticed in looking closer at the test failures in Gutenberg was that failures were coming from node_modules of individual workspace directories (e.g. gutenberg/packages/blocks/node_modules/memize/dist/index.js). So I think npm link to try to use the revisions in this branch wasn't working as I was expecting.

Eventually I ran npx lerna exec npm link memize to link to the local copy of Memize across all workspace packages and the tests seemed to pass, in Node.js 14, with the changes prior to 4a4dea2. Which makes me think that it actually does work, it's just that the workspaces weren't using the local copy of the package?

[johnhooks]

@aduth I will try this, I didn't consider lerna.

That worked, I left the process.env conditional in cjs, otherwise there is that error. We could break it, it would be a forcing factor to change the failing test. The updates to memize are required to move forward two types PRs for blocks and block-editor.

Curious how you were testing

As soon as I realized there was a problem I fell back to a clean fork of trunk. I have a branch prepared to update memize in all the packages that depend on it. I'm going to break it out from the types PR, that one is already very large.

[aduth]

I left the process.env conditional in cjs, otherwise there is that error.

Is this the affected code? https://github.com/WordPress/gutenberg/blob/488295de/packages/edit-site/src/store/test/utils.js#L147-L150

I really think it'd be best to propose to drop that, or refactor it so that it checks that the original function being memoized isn't called again on subsequent calls (e.g. mockFn.mock.calls.length). getCache is not part of the public API, and I don't want to support it.

Partly my concern is the inconsistency between ESM and CJS versions. I'm guessing previously it was not auto-replacing the ENV value except in the prebuilt browser version, so we probably could drop the replace altogether to restore that behavior, but it does seem like that requires that downstream projects replace NODE_ENV if they use memize in the browser, which is an unfortunate requirement to impose.

[gziolo]

Jest having poor support overall for ESM.

Yes, that’s the issue. They have a complex roadmap to get there that has some dependency on changes in Node.js itself and they collaborate between projects.

It's fine to leave ESM in the package configured without workarounds, if possible. We can list memize as ESM in Gutenberg. Here is the place for Jest changes:

https://github.com/WordPress/gutenberg/blob/trunk/test/unit/scripts/resolver.js

You can compare how react-colorful exports ESM and CJS:

https://unpkg.com/browse/react-colorful@5.6.1/package.json

or uuid:

https://unpkg.com/browse/uuid@9.0.0/package.json

it looks pretty complex!

[johnhooks]

@aduth yes that is the offending code. I added back the replacement of process.env in the cjs build. I 100% agree, let's fix the issue in Gutenberg, upgrading during a breaking change seems a perfect time.

@gziolo thank you for chiming in, I looked at test/unit/scripts/resolver.js, I don't think it will be necessary, the way the exports of memize are configured right now are working.

{
	"name": "memize",
	"version": "2.0.0",
	"description": "Unabashedly-barebones memoization library with an aim toward speed",
	"type": "module",
	"main": "dist/index.js",  // Since the type is module the main property is pointing to the ESM version.
	"exports": {
		"import": "./dist/index.js",
		"require": "./dist/index.cjs" // Jest is able to find this version
	},
	"types": "dist/index.d.ts",
	// ....
}

Edit:

I tested with Gutenberg, test pass (with that one commented out) and it builds successfully.

[gziolo]

I think the current setup makes sense, because when you define the type as module, there is no need to use .mjs extension.

[johnhooks]

@gziolo as I am testing this with Gutenberg (aside: boy, my poor computer is getting hot), I have some tests that seem to randomly timeout and fail, is that normal?

This is the common offender:

Summary of all failing tests
 FAIL  packages/block-library/src/cover/test/edit.js (73.475 s)
  ● Cover block › Inspector controls › Dimensions panel › sets minHeight attribute when number control value changed

    thrown: "Exceeded timeout of 5000 ms for a test.
    Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

      300 |
      301 |             describe( 'Dimensions panel', () => {
    > 302 |                     test( 'sets minHeight attribute when number control value changed', async () => {
          |                     ^
      303 |                             await setup();
      304 |                             await createAndSelectBlock();
      305 |                             await userEvent.click(

      at test (packages/block-library/src/cover/test/edit.js:302:4)
      at describe (packages/block-library/src/cover/test/edit.js:301:3)
      at describe (packages/block-library/src/cover/test/edit.js:132:2)
      at Object.describe (packages/block-library/src/cover/test/edit.js:33:1)

[gziolo]

@gziolo as I am testing this with Gutenberg (aside: boy, my poor computer is getting hot), I have some tests that seem to randomly timeout and fail, is that normal?

There is a nearly 600 test files so it might happen. It takes less than a minute to run them all on Mac with M1 😅

[johnhooks]

It takes less than a minute to run them all on Mac with M1

@gziolo I think I need to upgrade then 😂

[johnhooks]

Edit: Don't bother with this comment - I was chasing issues in the package-lock.json I think it's resolved now.

@aduth in WordPress/gutenberg#50172 with the current version 2.0.0 the Jest tests are passing. I think the original issue I had in WordPress/gutenberg#48604 was related to issue in the package-lock.json or not using the appropriate Node version when updating the version of memize.

EDIT: ^^ that wasn't right, the package-lock.json wasn't correct and installed memize v1.

There is a new problem related to the ES Module import in the lint CI workflow, I'm investigating it now.

https://github.com/WordPress/gutenberg/actions/runs/4831032473/jobs/8607946542?pr=50172#step:5:543

Something in the ESLint config must not like importing ES Modules, the rule import/no-unresolved is failing. IMHO t's something that should be fixed in Gutenberg, it's not an issue created by memize.

Edit:

I am getting very different results between CI and local... I think its issues with using npm link. I'm trying a fresh install of the dependencies and will try again.

[aduth]

Hey @gziolo 😄 Thanks for jumping in and pointing to that resolver.js file, glad to know that's an option.

It sounds like maybe we don't need to change anything in Memize then? But I'll wait to hear how the upgrade shakes out.

[johnhooks]

@aduth from what I understood, there still needs to be a CommonJS export, is that correct @gziolo? With the current additions of this PR, Gutenberg tests pass and it builds.

All the JavaScript test workflows are currently failing WordPress/gutenberg#50172

But I'm not very knowledgeable about the possibilities of fixing the problem from the Jest configuration side. It looks like it should be able to load ES Modules https://jestjs.io/docs/ecmascript-modules

[aduth]

Looks great 👍 I'll publish this shortly.

[aduth]

Published as v2.1.0. Thanks again @johnhooks !