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

no redeclaration of reserved __dirname #809

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Conversation

vlinder
Copy link
Contributor

@vlinder vlinder commented Jun 13, 2024

Running jest with this package crashes because of redeclaration of reserved keyword __dirname.

This PR renames it to dirname__.

@xenova
Copy link
Collaborator

xenova commented Jun 13, 2024

Hi there 👋 Can you provide the error logs you get (+ jest version)? We use jest for unit tests and do not have any issues with it.

@vlinder
Copy link
Contributor Author

vlinder commented Jun 13, 2024

We currently use version 29.7.0 of jest.

I guess jest can be configured in a lot of ways. I see no reason to fight against it if avoidable.

Here is the error message:

    Details:

    /Volumes/SourceCode/web/node_modules/.pnpm/@[email protected]/node_modules/@xenova/transformers/src/env.js:49
    const __dirname = RUNNING_LOCALLY ? _path.default.dirname(_path.default.dirname(_url.default.fileURLToPath(require("url").pathToFileURL(__filename).toString()))) : './';
          ^

    SyntaxError: Identifier '__dirname' has already been declared

I think it is because it converts the module to CommonJS.

This is what the node manual says:

The following variables may appear to be global but are not. They exist only in the scope of CommonJS modules:

So since they are set 'in scope' you cannot redeclare them.

I tried this with a minimal test file

const __dirname = 1;
$ node test.js
/Volumes/SourceCode/test.js:1
const __dirname = 1;
      ^

SyntaxError: Identifier '__dirname' has already been declared

@vlinder
Copy link
Contributor Author

vlinder commented Aug 21, 2024

Any blockers to merge this?

@TGTGamer
Copy link

TGTGamer commented Sep 2, 2024

@vlinder & @xenova this is also blocking drizzle-kit from completing successfully.

const __dirname = RUNNING_LOCALLY ? import_path.default.dirname(import_path.default.dirname(import_url.default.fileURLToPath(__esbuild_register_import_meta_url__))) : "./";
      ^

SyntaxError: Identifier '__dirname' has already been declared
    at wrapSafe (node:internal/modules/cjs/loader:1281:20)
    at Module._compile (node:internal/modules/cjs/loader:1321:27)
    at extensions..js (project\node_modules\.pnpm\[email protected]\node_modules\drizzle-kit\bin.cjs:17218:20)
    at Module.load (node:internal/modules/cjs/loader:1208:32)
    at Module._load (node:internal/modules/cjs/loader:1024:12)
    at Module.require (node:internal/modules/cjs/loader:1233:19)
    at require (node:internal/modules/helpers:179:18)
    at Object.<anonymous> (project\node_modules\.pnpm\@[email protected]\node_modules\@xenova\transformers\src\utils\hub.js:11:21)
    at Module._compile (node:internal/modules/cjs/loader:1358:14)
    at extensions..js (project\node_modules\.pnpm\[email protected]\node_modules\drizzle-kit\bin.cjs:17218:20)

@xenova xenova self-requested a review September 3, 2024 10:25
Copy link
Collaborator

@xenova xenova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay - PR looks good!

src/env.js Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@xenova xenova merged commit 50d5620 into huggingface:main Sep 3, 2024
1 of 4 checks passed
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.

4 participants