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.

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

convert to esm #265

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

convert to esm #265

wants to merge 5 commits into from

Conversation

talentlessguy
Copy link

@talentlessguy talentlessguy commented Mar 20, 2023 •
edited

This PR converts the source code to ESM, potentially fixing #171, esm-dev/esm.sh#553 and esm-dev/esm.sh#219

todo:

webworker tests are blocked by hugomrdias/playwright-test#530

@welcome
Copy link

welcome bot commented Mar 20, 2023

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines
    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

talentlessguy
talentlessguy commented Mar 20, 2023
View reviewed changes
package.json
@@ -13,10 +13,16 @@
"url": "https://github.com/ipfs/js-ipfs-utils/issues"
},
"engines": {
"node": ">=16.0.0",
"node": ">=16.8.0",
Copy link
Author

Choose a reason for hiding this comment

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

undici which is a dependency of native-fetch requires node.js 16.8+ to work: https://undici.nodejs.org/#/?id=undicifetchinput-init-promise

package.json
},
"devDependencies": {
"aegir": "^36.1.1",
"aegir": "^38.1.7",
Copy link
Author

Choose a reason for hiding this comment

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

to support es module in testing

src/fetch.js
// use window.fetch if it is available, fall back to node-fetch if not
let impl = 'native-fetch'

if (isElectronMain) {
impl = 'electron-fetch'
}

module.exports = require(impl)
const fetchImpl = await import(impl)
Copy link
Author

Choose a reason for hiding this comment

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

replace dynamic require with dynamic import-s (top level await is available since node v14.18, dynamic import is supported by v13.2)

tsconfig.json
@@ -2,7 +2,9 @@
"extends": "aegir/src/config/tsconfig.aegir.json",
"compilerOptions": {
"outDir": "dist",
"emitDeclarationOnly": true
"emitDeclarationOnly": true,
"module": "ES2022",
Copy link
Author

Choose a reason for hiding this comment

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

dynamic import requires module to be ES2022 but it only impacts type checks and declarations, it doesn't apply to source code

@talentlessguy talentlessguy mentioned this pull request Mar 20, 2023
Open
@talentlessguy talentlessguy marked this pull request as draft March 20, 2023 12:59
talentlessguy
talentlessguy commented Mar 20, 2023
View reviewed changes
.aegir.js
Copy link
Author

Choose a reason for hiding this comment

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

atm it fails bc of esbuild:

[15:19:06] esbuild [started]
[15:19:06] esbuild [completed]
test webworker
⠙ Setting up chromiumBEWARE: your OS is not officially supported by Playwright; downloading fallback build.
BEWARE: your OS is not officially supported by Playwright; downloading fallback build.
✔ chromium set up
✘ [ERROR] No matching export in "(disabled):src/files/glob-source.js" for import "default"

    test/files/glob-source.spec.js:4:7:
      4 │ import globSource from '../../src/files/glob-source.js'
        ╵        ~~~~~~~~~~

✖ Running tests failed.
Error: Build failed with 1 error:
test/files/glob-source.spec.js:4:7: ERROR: No matching export in "(disabled):src/files/glob-source.js" for import "default"
    at failureErrorWithLog (/home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/esbuild@0.16.10/node_modules/esbuild/lib/main.js:1596:15)
    at /home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/esbuild@0.16.10/node_modules/esbuild/lib/main.js:1052:28
    at runOnEndCallbacks (/home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/esbuild@0.16.10/node_modules/esbuild/lib/main.js:1468:61)
    at buildResponseToResult (/home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/esbuild@0.16.10/node_modules/esbuild/lib/main.js:1050:7)
    at /home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/esbuild@0.16.10/node_modules/esbuild/lib/main.js:1162:14
    at responseCallbacks.<computed> (/home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/esbuild@0.16.10/node_modules/esbuild/lib/main.js:697:9)
    at handleIncomingPacket (/home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/esbuild@0.16.10/node_modules/esbuild/lib/main.js:752:9)
    at Socket.readFromStdout (/home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/esbuild@0.16.10/node_modules/esbuild/lib/main.js:673:7)
    at Socket.emit (node:events:394:28)
    at addChunk (node:internal/streams/readable:315:12) {
  errors: [
    {
      detail: undefined,
      id: '',
      location: [Object],
      notes: [],
      pluginName: '',
      text: 'No matching export in "(disabled):src/files/glob-source.js" for import "default"'
    }
  ],
  warnings: []
}
Command failed with exit code 1: /home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/aegir@38.1.7_eslint-plugin-n@15.6.1/node_modules/aegir/node_modules/.bin/pw-test test/**/*.spec.{js,cjs,mjs} test/browser.{js,cjs,mjs} dist/test/**/*.spec.{js,cjs,mjs} dist/test/browser.{js,cjs,mjs} --mode worker --config /home/v1rtl/Coding/forks/js-ipfs-utils/node_modules/.pnpm/aegir@38.1.7_eslint-plugin-n@15.6.1/node_modules/aegir/src/config/pw-test.js --timeout=60000 --bail
 ELIFECYCLE  Command failed with exit code 1.

Copy link
Author

Choose a reason for hiding this comment

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

The reason might be this: evanw/esbuild#2780 or this: hugomrdias/playwright-test#530

@talentlessguy talentlessguy mentioned this pull request Mar 20, 2023
Closed
@talentlessguy talentlessguy changed the title convert to esm and bump dependencies so it works with esm convert to esm Mar 20, 2023
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.

None yet
1 participant
@talentlessguy