Skip to content

fix: node-next and the weird case of importing ts code from js files#52

Closed
whizzzkid wants to merge 4 commits intomainfrom
fix/node-next
Closed

fix: node-next and the weird case of importing ts code from js files#52
whizzzkid wants to merge 4 commits intomainfrom
fix/node-next

Conversation

@whizzzkid
Copy link
Collaborator

In this PR:

  • I don't think nodenext is ready for primetime, it results in cognitive dissonance and poor DX.
  • There is an entire thread: "module": "node16" should support extension rewriting microsoft/TypeScript#49083 where devs just vetoed based on philosophy rather than what's ergonomic.
  • It just feels weird import TS code from JS files and believe it's true.
  • Also, index files should be resolved automatically so fixed that too.

@SgtPooki
Copy link
Member

While I would like to be able to import foo from './myTypescript.ts' instead of import foo from './myTypescript.js', I do not believe this is "fixing" anything in this repo. Please let me know if I'm missing something, but this is changing things further from the patterns already set for a majority of the js-ipfs projects.

Many of the comments in the linked issue seem to be about deno and interop support, or migrating of existing projects.. but things are already working in this repo when building, and when imported into ESM projects.

What is this fixing? what was broken?

@whizzzkid whizzzkid marked this pull request as draft January 18, 2023 00:01
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

when i run this against the companion PR, I get:

╰─ ✘ 1 ❯ npx mocha --timeout 5000 --exit --require ignore-styles --require @babel/register "test/functional/**/*.test.js" "-g" "should query local storage for options with hardcoded defaults for fallback"


  lib/ipfs-companion.js
    init
      1) should query local storage for options with hardcoded defaults for fallback


  0 passing (59ms)
  1 failing

  1) lib/ipfs-companion.js
       init
         should query local storage for options with hardcoded defaults for fallback:
     Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/sgtpooki/code/work/protocol.ai/ipfs/ipfs-companion/node_modules/@ipfs-shipyard/ignite-metrics/dist/src/config' imported from /Users/sgtpooki/code/work/protocol.ai/ipfs/ipfs-companion/node_modules/@ipfs-shipyard/ignite-metrics/dist/src/MetricsProvider.js
      at new NodeError (node:internal/errors:371:5)
      at finalizeResolution (node:internal/modules/esm/resolve:394:11)
      at moduleResolve (node:internal/modules/esm/resolve:944:10)
      at defaultResolve (node:internal/modules/esm/resolve:1041:11)
      at ESMLoader.resolve (node:internal/modules/esm/loader:530:30)
      at ESMLoader.getModuleJob (node:internal/modules/esm/loader:251:18)
      at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:79:40)
      at link (node:internal/modules/esm/module_job:78:36)

@SgtPooki
Copy link
Member

SgtPooki commented Jan 18, 2023

when I add the extension to config.js in MetricsProvider, it resolves the config file fine, but still can't resolve the countly-web-sdk dependency (same error with ipfs/ipfs-companion#1117):

Run `npm audit` for details.


  lib/ipfs-companion.js
    init
      1) should query local storage for options with hardcoded defaults for fallback


  0 passing (857ms)
  1 failing

  1) lib/ipfs-companion.js
       init
         should query local storage for options with hardcoded defaults for fallback:
     TypeError: Cannot read properties of undefined (reading 'init')
      at new MetricsProvider (file:///Users/sgtpooki/code/work/protocol.ai/ipfs/ipfs-companion/node_modules/@ipfs-shipyard/ignite-metrics/dist/src/MetricsProvider.js:14:29)
      at getMetricsProviderInstance (file:///Users/sgtpooki/code/work/protocol.ai/ipfs/ipfs-companion/add-on/src/lib/telemetry.js:9:40)
      at startSession (file:///Users/sgtpooki/code/work/protocol.ai/ipfs/ipfs-companion/add-on/src/lib/telemetry.js:67:42)
      at Module.init (file:///Users/sgtpooki/code/work/protocol.ai/ipfs/ipfs-companion/add-on/src/lib/ipfs-companion.js:62:7)
      at async Context.<anonymous> (file:///Users/sgtpooki/code/work/protocol.ai/ipfs/ipfs-companion/test/functional/lib/ipfs-companion.test.js:37:29)

Comment on lines +9 to +11
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"moduleResolution": "Node"
Copy link
Member

Choose a reason for hiding this comment

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

These lines are redundant, they all set the same values as in the extendedtsconfig.aegir.json - https://github.com/ipfs/aegir/blob/master/src/config/tsconfig.aegir.json#L22

"types": ["node"],
"outDir": "dist",
"target": "ES6",
"target": "ESNext",
Copy link
Member

Choose a reason for hiding this comment

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

aegir's config has a es2020 target, setting it to ESNext is a bit yolo - you'll get whatever the latest version the current tsc supports is.

This is es2022 at the moment, are there features in that which are required?

@@ -1,4 +1,4 @@
import { COUNTLY_API_URL } from './config.js'
import { COUNTLY_API_URL } from './config'
Copy link
Member

Choose a reason for hiding this comment

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

If the published output is ESM, you need the extension for loading individual files, and you can't import from a directory without /index.js. This different to outputting CJS which will work without extensions etc as before.

Tediously this breakage only shows up at runtime which I think is what @SgtPooki has discovered testing this with companion.

@SgtPooki
Copy link
Member

discussion held in ipfs-gui-dev chat at https://filecoinproject.slack.com/archives/C03KQ8MC62Y/p1674001285847779. Closing

@SgtPooki SgtPooki closed this Jan 18, 2023
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.

3 participants