8000 Convert tests from CommonJS to ES Module syntax by eliot-akira · Pull Request #2076 · isomorphic-git/isomorphic-git · GitHub
[go: up one dir, main page]
Skip to content

Convert tests from CommonJS to ES Module syntax #2076

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Convert tests from CommonJS to ES Module syntax #2076

wants to merge 7 commits into from

Conversation

eliot-akira
Copy link
Contributor
@eliot-akira eliot-akira commented Apr 24, 2025 •
edited
Loading

This pull request converts the tests from CommonJS to ES Module syntax.

CommonJS can be recognized by the use of the require() function and module.exports, while ES modules use import and export statements

It's part of a set of changes that created a fork of path-browserify with modernized codebase and tree-shakeable exports (ESM); upgrade TypeScript in the build setup and supported browser matrix; update Jest config and convert everything including tests to ES Module so the new fork can be tested together.

After this PR is refined and merged, #2035 should fast-forward/merge from main branch to complete its final step. When all is done, isomorphic-git should be usable from JavaScript runtimes such as Deno and Bun.

@jcubic

@eliot-akira
Copy link
Contributor Author
eliot-akira commented Apr 24, 2025 •
edited
Loading

The tests are now failing with the same error as #2004.

error: Uncaught SyntaxError: The requested module 'path-browserify' does not provide an export named 'join'

This is expected behavior because the original path-browserify package does not export methods individually, only as an object in default export using CommonJS. So the below statement fails:

import { join } from 'path-browserify'

I'll fix this by correcting the import statements to destructure the join method from default.

import path from 'path-browserify'

const { join } = path

This is only temporary until #2035 is completed, which switches this library to use the newly created @isomorphic-git/path-browserify that exports its methods individually using ES Module syntax.

@eliot-akira
Destructure path.join until isomorphic-git#2035 is completed: origi…
15f7967 
…nal `path-browserify` only has default export in CommonJS
@eliot-akira
Copy link
Contributor Author
eliot-akira commented Apr 24, 2025 •
edited
Loading

Next to be solved, this use of await import('fs').

ReferenceError: You are trying to `import` a file after the Jest environment has been torn down. From __tests__/test-log-file.js.

       7 |
       8 | export async function makeNodeFixture(fixture) {
    >  9 |   const _fs = Object.assign({}, await import('fs'))
         |                                 ^
      10 |
      11 |   const fs = new FileSystem(_fs)
      12 |

      at makeNodeFixture (__tests__/__helpers__/FixtureFS/makeNodeFixture.js:9:33)
      at makeNodeFixture (__tests__/__helpers__/FixtureFS.js:10:54)
      at Object.makeFixture (__tests__/test-readObject.js:119:34)

Link

From the message, it may be possible to work around it by importing fs at the top level, instead of a dynamic import.

A similar error elsewhere.

Error [ERR_VM_MODULE_NOT_MODULE]: Provided module is not an instance of Module

       7 |
       8 | export async function makeNodeFixture(fixture) {
    >  9 |   const _fs = Object.assign({}, await import('fs'))
         |                                 ^
      10 |
      11 |   const fs = new FileSystem(_fs)
      12 |

      at makeNodeF
8000
ixture (__tests__/__helpers__/FixtureFS/makeNodeFixture.js:9:33)
      at Object.<anonymous> (__tests__/test-push.js:21:28)

Link

Not sure what it means. In a REPL, await import('fs') does return an instance of Module. Maybe (await import('fs')).default is necessary, I've encountered that before.

Weirdly I'm not seeing these errors when I run the tests locally. It may depend on Node.js version.

@jcubic
Copy link
Member
jcubic commented Apr 24, 2025

I can't merge when there are failing tests. If you want to work on this when a PR may break things, I can create a branch for you that you will merge stuff into. When the code will be ready, we will merge that branch as one thing. Let me know if this will work.

@eliot-akira
Copy link
Contributor Author

Sure, understood. I'm still working through the errors, I'll see if they can be completely solved in this PR by itself while still using the original path-browserify.

@acandoo acandoo mentioned this pull request Jun 24, 2025
Open
This was referenced Aug 19, 2025
Open
Closed
@lemanschik
Copy link

@eliot-akira we should consider not forking it

We Should maybe drop the browserify path implementations in favor of URL as even NodeJS Promise fileSystem methods do support URL and so...

see:

@eliot-akira
Copy link
Contributor Author

#2169 looks great, much simpler to rely on native URL. I guess as long as it passes all existing tests and is supported by the oldest Node.js version supported by this library (or bump up the requirement).

If that PR lands, the change could be pulled into this one and hopefully it will make the ESM conversion easier.

@lemanschik
Copy link

@eliot-akira do not forget we can always Polyfill backward later eg supply URL but i vote for node 18 anyway as smallest target even for tests and we need to get them passing with node 18 anyway. But Readme States that this then needs version bumb to v2 as it is breaking compatibility matrix but that needs to get done anyway even out of security view this needs to get tested on newst chrome and then we later supply backward compat.

@jcubic
Copy link
Member
jcubic commented Aug 22, 2025 •
edited
Loading

I don't think that we need to bump major version after dropping the old Node. We only need to update everywhere that we don't support them anymore.

@lemanschik
Copy link

@jcubic good point in real world we would just do 1.xx+1 as we do not real break the existing API and only do additions. But we should also update the readme with that update as it states clear as soon as version matrix of support is broken we release major 1+1.xxxx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@eliot-akira @jcubic @lemanschik
0