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

add named export to improve "module": "nodenext" compatibility #140

Merged
merged 6 commits into from
Jul 23, 2022

Conversation

wight554
Copy link
Contributor

* currently it shows error:
This expression is not callable.
  Type 'typeof import("node_modules/vite-plugin-checker/lib/main")' has no call signatures.ts(2349)
* see microsoft/TypeScript#48845
@wight554
Copy link
Contributor Author

it's possible to follow this example as an alternative but that requires deleting isObject export from main file (is it needed tho?)

@wight554
Copy link
Contributor Author

@fi3ework any objections on merging this one?
Fixes this issue:
image

@fi3ework
Copy link
Owner

@wight554 thanks for the PR, I was wondering is there way to also use default export in ESM which will be aligned with CJS?

@wight554
Copy link
Contributor Author

@wight554 thanks for the PR, I was wondering is there way to also use default export in ESM which will be aligned with CJS?

Ideally .d.ts for CJS and .d.mts for ESM, so CJS file definition can have exported namespace, like in example.
But I think Typescript compiler doesn't create separate type definitions from .ts files

@fi3ework
Copy link
Owner

Does a conditional export works for this? https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-rc/#package-json-exports-imports-and-self-referencing ESM and CJS can use different types entries and we can build different types fils before publishing

@wight554
Copy link
Contributor Author

Does a conditional export works for this? https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-rc/#package-json-exports-imports-and-self-referencing ESM and CJS can use different types entries and we can build different types fils before publishing

Well having mjs typings requires mjs module, won't it be overhead ti to have 2 versions if plugin if it works as is?
It's just esm requires named cjs exports to work properly with cjs modules

@netlify
Copy link

netlify bot commented Jul 22, 2022

Deploy Preview for vite-plugin-checker ready!

Name Link
🔨 Latest commit f29e09a
🔍 Latest deploy log https://app.netlify.com/sites/vite-plugin-checker/deploys/62dad63cd930a20008438a63
😎 Deploy Preview https://deploy-preview-140--vite-plugin-checker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@wight554
Copy link
Contributor Author

wight554 commented Jul 22, 2022

@fi3ework added changes to make module cjs style, can you build and attach output .d.ts file?
Unfortunately I couldn't get build command run successfully on 2 machines, always fails with:

CLI Building entry: src/Checker.ts, src/FileDiagnosticManager.ts, src/codeFrame.ts, src/logger.ts, src/main.ts, src/types.ts, src/worker.ts, src/client/index.ts, src/checkers/eslint/cli.ts, src/checkers/eslint/main.ts, src/checkers/eslint/options.ts, src/checkers/typescript/main.ts, src/checkers/vls/cli.ts, src/checkers/vls/diagnostics.ts, src/checkers/vls/initParams.ts, src/checkers/vls/main.ts, src/checkers/vls/typings.d.ts, src/checkers/vueTsc/main.ts, src/checkers/vueTsc/prepareVueTsc.ts
CLI Using tsconfig: tsconfig.json
CLI tsup v5.12.3
CLI Using tsup config: /Users/Volodymyr_Zhdanov/playground/vite-plugin-checker/packages/vite-plugin-checker/tsup.config.ts
CLI Target: node12
CJS Build start
post-esbuild bundling patch failed
 ELIFECYCLE  Command failed with exit code 1.

@fi3ework
Copy link
Owner

I'm not sure if export = plugin is a valid ES syntax, if so, could you please paste a link for that?

@fi3ework
Copy link
Owner

I think the previous implementation is simple enough to handle the nodenext for now.

Does a conditional export works for this? https://devblogs.microsoft.com/typescript/announcing-typescript-4-7-rc/#package-json-exports-imports-and-self-referencing ESM and CJS can use different types entries and we can build different types fils before publishing

Well having mjs typings requires mjs module, won't it be overhead ti to have 2 versions if plugin if it works as is? It's just esm requires named cjs exports to work properly with cjs modules

I think you're right. Maybe we can export consistently of CJS and ESM in the next minor version. Adding another export is good enough for now. Would like to revert to the prev version and make it merged. Thanks!

@wight554
Copy link
Contributor Author

I'm not sure if export = plugin is a valid ES syntax, if so, could you please paste a link for that?

It's valid for CJS, but yeah previous version was cleaner

@fi3ework
Copy link
Owner

I'm not sure if export = plugin is a valid ES syntax, if so, could you please paste a link for that?

It's valid for CJS, but yeah previous version was cleaner

We hope to resolve this in a more ESM flavor way as it's becoming more and more mainstream.

@wight554
Copy link
Contributor Author

revert to the prev version

Done

@fi3ework
Copy link
Owner

There's an extra semicolon that fails the CI 👀

@wight554
Copy link
Contributor Author

There's an extra semicolon that fails the CI 👀

fixed

@fi3ework
Copy link
Owner

fi3ework commented Jul 22, 2022

const modifiedCode = patchEsbuildDist(targetFile.text, 'Plugin')

'Plugin' also should be changed to 'checker'

@wight554
Copy link
Contributor Author

/packages/vite-plugin-checker/tsup.config.ts

done

@fi3ework fi3ework merged commit dca4157 into fi3ework:main Jul 23, 2022
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.

2 participants