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

Update tsconfig.json to use use node16 module resolution #123

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

ChristianMurphy
Copy link
Member

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Additionally scopes JSON module to the one (CJS) location where it is used.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jan 16, 2023
@wooorm
Copy link
Member

wooorm commented Jan 16, 2023

Where is JSON still required? 🤔

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2023

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (c898b77) compared to base (b9e9b99).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #123   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          460       460           
=========================================
  Hits           460       460           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ChristianMurphy
Copy link
Member Author

Where is JSON still required? 🤔

const proc = require('rehype/package.json')
const cli = require('./package.json')

tsconfig.json Outdated Show resolved Hide resolved
@wooorm
Copy link
Member

wooorm commented Jan 16, 2023

Ohh yeah... That's been a while. I'll probably hardcode it soon. Weird that ts doesn't understand you can do that with cjs

@ChristianMurphy
Copy link
Member Author

Weird that ts doesn't understand you can do that with cjs

Well this file technically isn't CJS, it's ESM, with a custom CJS resolve defined inside it 😅

const require = createRequire(import.meta.url)

@remcohaszing
Copy link
Member

It kind of understands. With "module": "node16", the following TypeScript:

import foo = require('bar')

gets compiled to:

import { createModule } from 'node:module'

const require = createModule(import.meta.url)

const foo = require('bar')

But indeed, it doesn’t understand if this is written manually, whether in JS or TS..

@remcohaszing
Copy link
Member

Still, I think resolveJsonModule isn’t needed to be able to require JSON files like that?

@ChristianMurphy
Copy link
Member Author

ChristianMurphy commented Jan 16, 2023

Still, I think resolveJsonModule isn’t needed to be able to require JSON files like that?

I agree it likely shouldn't be required.
But it currently is.
Removing the option results in:

cli.js:8:22 - error TS2732: Cannot find module 'rehype/package.json'. Consider using '--resolveJsonModule' to import module with '.json' extension.

8 const proc = require('rehype/package.json')
                       ~~~~~~~~~~~~~~~~~~~~~

cli.js:9:21 - error TS2732: Cannot find module './package.json'. Consider using '--resolveJsonModule' to import module with '.json' extension.

9 const cli = require('./package.json')
                      ~~~~~~~~~~~~~~~~

@wooorm wooorm changed the title types: use node16 module resolution in typescript Update tsconfig.json to use use node16 module resolution Jan 17, 2023
@wooorm wooorm merged commit 864e813 into rehypejs:main Jan 17, 2023
@github-actions

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Jan 17, 2023

Well this file technically isn't CJS, it's ESM, with a custom CJS resolve defined inside it 😅

It knows that it is a require call right, as it resolves things from it. Presumably it can differentiate between imports and requires. So I don’t understand why it treats it as if it would use “ESM”/import semantics instead of “CJS”/require ones

@wooorm wooorm added ☂️ area/types This affects typings 💪 phase/solved Post is done labels Jan 17, 2023
@wooorm
Copy link
Member

wooorm commented Jan 17, 2023

Thanks!

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jan 17, 2023
@remcohaszing
Copy link
Member

I agree it likely shouldn't be required.
But it currently is.

I really thought it was required, but shouldn’t be! Anyway, I have no further comments if it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☂️ area/types This affects typings 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants