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

chore: fix esm tests #1742

Draft
wants to merge 4 commits into
base: test/use-esm
Choose a base branch
from

Conversation

mallachari
Copy link
Contributor

@mallachari mallachari commented Sep 24, 2024

What/Why/How?

This PR attempts to solve issues encountered after ESM migration, mostly due to incompatibility of commonjs modules (that are being used) with type definitions and they way how ESM is accessing them.

Identified problems:

  • module resolutions - js modules imports were replaced with .js in the original commit which caused typescript to incorrectly assume that it's not a module. The fix for that is adding moduleNameMapper to jest config to map these modules
  • node-fetch default import causing missinterpretation of module structure by typescript leading to use of
import _fetch from 'node-fetch';
const fetch = _fetch.default;

This probably happens because there is an old version of node-fetch (2.6.1) which is a commonjs module, that uses its own type definitions. These definitions are incompatible with the global fetch types from @types/node. The esModuleInterop flag helps but doesn’t resolve the conflict caused by duplicate fetch definitions.

I've bumped node-fetch to v3 which solves the issue, but requires slight adjustments to typings.

  • There is a similar problem with ajv lib (@redocly/ajv/dist/2020.js), also commonjs module, that is being accessed the same way. In that case bumping version might not be an option, so it requires different approach (that I failed to figure out).
  • jest config required additional tweaks - apart from mapping modules, it needed additional transform to ensure ts-jest correctly handles ESM and import assertions.
  • colorette - there are missing type declarations for this package which is outdated. Upgrading it to newer version would fix it but it introduces breaking change for colorette.options that don't exist anymore, but is used throughout the code. A way to walk-around this (and what I ended up doing) is to add type definition file manually.
  • There is an unexpected with token usage that is breaking tests. My assumption is that it might be caused by import packageJson from '../../package.json' with { type: 'json' }, however I wasn't able to walk around it and not sure if there isn't some other with occurrence that I missed
  • Running tests may require using node --experimental-vm-modules

Reference

Testing

Screenshots (optional)

Check yourself

  • Code changed? - Tested with redoc/reference-docs/workflows (internal)
  • All new/updated code is covered with tests
  • New package installed? - Tested in different environments (browser/node)

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@mallachari mallachari requested a review from a team as a code owner September 24, 2024 14:27
Copy link

changeset-bot bot commented Sep 24, 2024

⚠️ No Changeset found

Latest commit: 500ff43

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@tatomyr
Copy link
Contributor

tatomyr commented Sep 24, 2024

Thanks for the fixes and for explanations! I'll take a deeper look into this once I have time.
Regarding tests, I nearly gave up and started leaning towards migrating away from Jest.

@mallachari
Copy link
Contributor Author

Moving away from jest might sometimes be the way to go, at least that is what I experience once when migrating to node 18 and switching to vitest.

@tatomyr
Copy link
Contributor

tatomyr commented Sep 25, 2024

For some reason the cli-package-test action which used to work in the original PR is failing (and some other actions too). That's strange because it passes locally (as far as I can judge from a quick test). It needs further investigation before merging.

@mallachari
Copy link
Contributor Author

Right, I see that. I focused mainly on figuring out loading certain modules. Definitely it requires further investigation, it should probably be draft pr instead.

@tatomyr
Copy link
Contributor

tatomyr commented Sep 25, 2024

NP, I'll convert it.

@tatomyr tatomyr marked this pull request as draft September 25, 2024 07:11
@mallachari
Copy link
Contributor Author

Ok, looks like I fixed those cli tests, at least it runs for me locally.
The problem was the import of package.json using with keyword. Additionally, the __dirname in esm should be accessed differently.
Even having that working, this PR should still be in draft until other tests get resolved

@tatomyr
Copy link
Contributor

tatomyr commented Sep 26, 2024

Thank you! I think I'll get rid of direct importing package.json as the approach causes more issues than resolves. Maybe I'll just read it from fs or fetch.

@mallachari
Copy link
Contributor Author

Another approach could be having a specific, accessible during runtime, version const that would be auto-updated during build process. Something like:

// auto-updated during build
export const appVersion = '1.0.0';

and in package.json scripts additional build step (using replace package or sed):

"version.set": "replace \"appVersion = '.*'\" \"appVersion = '$npm_package_version'\" src/version.ts --silent"

But importing package.json directly with that assert keyword might be good enough I think

@tatomyr
Copy link
Contributor

tatomyr commented Sep 26, 2024

importing package.json directly with that assert keyword might be good enough I think

Unfortunately it still fails in Docker env: https://github.com/Redocly/redocly-cli/actions/runs/11032319745/job/30642998846?pr=1742 😅

@tatomyr tatomyr mentioned this pull request Nov 7, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants