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

Cannot use import statement outside a module (ES, CommonJS) #4347

Closed
BLCK-B opened this issue Aug 13, 2024 · 9 comments · Fixed by #4350
Closed

Cannot use import statement outside a module (ES, CommonJS) #4347

BLCK-B opened this issue Aug 13, 2024 · 9 comments · Fixed by #4350
Labels
A-Developer-Experience O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect

Comments

@BLCK-B
Copy link
Contributor

BLCK-B commented Aug 13, 2024

I tried to import matrix-js-sdk both in CommonJs and ES projects.

(node:22992) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)

import * as matrixcs from "./matrix";
SyntaxError: Cannot use import statement outside a module
    at wrapSafe (node:internal/modules/cjs/loader:1281:20)
    at Module._compile (node:internal/modules/cjs/loader:1321:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1416:10)
    at Module.load (node:internal/modules/cjs/loader:1208:32)
    at Module._load (node:internal/modules/cjs/loader:1024:12)
    at cjsLoader (node:internal/modules/esm/translators:348:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:297:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:123:5)

The reported fixes, type module and .mjs, do not work.

To reproduce:
https://github.com/BLCK-B/MatrixSDKissue

  • cd theCode
  • npm install
  • npm start OR alternatively node app.mjs

Node.js v20.16.0

It reportedly does not work in tsnode either (Ho Ching):
image

Likely related:
#4284
#3145

@richvdh
Copy link
Member

richvdh commented Aug 13, 2024

Thanks for the report.

This is actually the same thing as #4284: my "fix" in #4285 just didn't work. If you open package.json in https://www.npmjs.com/package/matrix-js-sdk/v/34.2.0?activeTab=code, you can see that there is no top-level "type": "module" setting.

I think the problem is that there are actually two scripts that modify package.json for the release process. scripts/release/pre-release.sh is the one that is actually used during release. scripts/switch_package_to_release.js is only used in CI. I have no idea why we need two of them (I don't even really know why we need one, to be honest.)

Anyway, TL;DR: let's update scripts/release/pre-release.sh in the same way that #4285 did. Then let's remove scripts/switch_package_to_release.js and just use scripts/release/pre-release.sh in CI.

@BLCK-B
Copy link
Contributor Author

BLCK-B commented Aug 13, 2024

I found that the Release Process workflow does not seem to trigger the Static Analysis, which in turn is the only trigger of switch_package_to_release.js, that one contains your fix.
Static Analysis:

on:
    pull_request: {}
    merge_group:
        types: [checks_requested]
    push:
        branches: [develop, master]

As for the two scripts, they seem to do the same thing. The pre-release.sh is run by Release Process.
The scripts could be merged with unified triggers. But before that, I think the solution is to add your fix pkgJson["type"] = "module" to pre-release.sh.
Why the ES module declaration is not there in the first place, I don't know.

@richvdh
Copy link
Member

richvdh commented Aug 13, 2024

Why the ES module declaration is not there in the first place, I don't know.

In short, because there are a bunch of other scripts in the git repository that aren't ES modules, and adding it breaks them. But yes, fixing those scripts and adding the declaration to package.json in git would be a better solution.

@dbkr dbkr added O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround labels Aug 15, 2024
@richvdh
Copy link
Member

richvdh commented Aug 15, 2024

Hopefully this is fixed by #4350. We'll see after the next RC

richvdh added a commit that referenced this issue Aug 20, 2024
matrix-js-sdk is built into ECMAScript modules, and we should declare it as
such. See https://nodejs.org/api/packages.html#type. Failure to do so causes
problems for javascript projects attempting to build against matrix-js-sdk: see #4347.

Previously, we did this as part of the package.json switcheroo, but that is
unnecessarily fragile.

matrix-react-sdk, element-web, etc are unaffected by this, because they use the
typescript files directly, by importing `matrix-js-sdk/src/...`.
@richvdh richvdh linked a pull request Aug 20, 2024 that will close this issue
4 tasks
richvdh added a commit that referenced this issue Aug 20, 2024
matrix-js-sdk is built into ECMAScript modules, and we should declare it as
such. See https://nodejs.org/api/packages.html#type. Failure to do so causes
problems for javascript projects attempting to build against matrix-js-sdk: see #4347.

Previously, we did this as part of the package.json switcheroo, but that is
unnecessarily fragile.

matrix-react-sdk, element-web, etc are unaffected by this, because they use the
typescript files directly, by importing `matrix-js-sdk/src/...`.
github-merge-queue bot pushed a commit that referenced this issue Aug 20, 2024
* Rename `switch_package_to_release.js` to `.cjs`

Slightly surprisingly, the symlink is enough to make `node
switch_package_to_release.js` work.

* Rename .eslintrc.js to .cjs

Again, declare this as commonjs

* Move `type:module` declaration into package.json.

matrix-js-sdk is built into ECMAScript modules, and we should declare it as
such. See https://nodejs.org/api/packages.html#type. Failure to do so causes
problems for javascript projects attempting to build against matrix-js-sdk: see #4347.

Previously, we did this as part of the package.json switcheroo, but that is
unnecessarily fragile.

matrix-react-sdk, element-web, etc are unaffected by this, because they use the
typescript files directly, by importing `matrix-js-sdk/src/...`.
@BLCK-B
Copy link
Contributor Author

BLCK-B commented Aug 20, 2024

I see the PR caused the workflow to fail. Then richvdh tried to add it directly? Right now, it is in code on git, but not on npm.
If you manage to fix it, I'd like to know how.

@richvdh
Copy link
Member

richvdh commented Aug 20, 2024

We're still working on this. We'll cut another RC tomorrow.

dbkr pushed a commit that referenced this issue Aug 21, 2024
* Rename `switch_package_to_release.js` to `.cjs`

Slightly surprisingly, the symlink is enough to make `node
switch_package_to_release.js` work.

* Rename .eslintrc.js to .cjs

Again, declare this as commonjs

* Move `type:module` declaration into package.json.

matrix-js-sdk is built into ECMAScript modules, and we should declare it as
such. See https://nodejs.org/api/packages.html#type. Failure to do so causes
problems for javascript projects attempting to build against matrix-js-sdk: see #4347.

Previously, we did this as part of the package.json switcheroo, but that is
unnecessarily fragile.

matrix-react-sdk, element-web, etc are unaffected by this, because they use the
typescript files directly, by importing `matrix-js-sdk/src/...`.
@richvdh
Copy link
Member

richvdh commented Aug 22, 2024

sorry, forgot to update: matrix-js-sdk v34.4.0-rc.1 was published yesterday, and this problem appears to be solved.

@BLCK-B's repro example now hits #4287, but that is a separate problem.

@richvdh richvdh closed this as completed Aug 22, 2024
@lukasvaz
Copy link

lukasvaz commented Sep 5, 2024

im hitting this problem (#4287 ) in v34 , i started developing just downgrading it to v32. Is there any solution available soon?.

@richvdh
Copy link
Member

richvdh commented Sep 5, 2024

Try v34.5.0-rc.0

This should be fine in v34.4. If not, open a new issue explaining in more detail what your problem is, and giving reproduction steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Developer-Experience O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants