Skip to content

Update#2

Merged
SunsetTechuila merged 9 commits intomasterfrom
update
Mar 23, 2025
Merged

Update#2
SunsetTechuila merged 9 commits intomasterfrom
update

Conversation

@SunsetTechuila
Copy link
Copy Markdown
Owner

CC: @JounQin

@SunsetTechuila SunsetTechuila requested a review from Copilot March 22, 2025 19:46
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the library by reorganizing the source code into separate modules (shared, generic, and bun) and updating tests and documentation to reflect the new API structure.

  • New tests and API functions for checking Bun modules, implemented Node modules, and built-in modules have been introduced.
  • The build configuration (tsup.config.ts) and README have been updated to match these changes.
  • Legacy code (src/index.ts and tests for deprecated functions) has been removed.

Reviewed Changes

Copilot reviewed 12 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/isBunBuiltin.test.ts Added tests for the new isBunBuiltin module checking function.
test/isBunImplementedNodeModule.test.ts Added tests for verifying Node modules implemented in Bun.
test/getModules.test.ts Introduced tests for module listing functions.
src/shared.ts New shared helpers for semver and module checking functionality.
src/generic.ts Exports generic module checking and listing functions.
src/bun.ts Implements Bun‑specific functions using Bun.version as default.
README.md Updated API documentation to reflect the reorganized functions.
tsup.config.ts Updated bundler configuration to support the new module structure.
test/bunVersion.test.ts Updated version parsing tests to use new function names.
test/versionRanges.test.ts Adjusted asset reference for implemented node modules.
src/index.ts Removed legacy index file.
test/isSupportedModule.test.ts Removed deprecated tests for legacy module support.
Files not reviewed (3)
  • package.json: Language not supported
  • src/assets/implemented-node-modules.json: Language not supported
  • src/assets/node-modules.json: Language not supported

@SunsetTechuila SunsetTechuila force-pushed the update branch 3 times, most recently from f24bcd8 to 84498e6 Compare March 23, 2025 17:50
Comment thread src/bun.ts Outdated
Comment thread src/generic.ts
* @returns `true` if the module is a Bun module, `false` otherwise
*/
export function isBunModule(moduleName: string, bunVersion?: BunVersion): boolean {
return checkModule(moduleName, bundledBunModules, bunVersion ?? "latest");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Try process.versions.bun first then fallack to latest? Maybe extract as const DEFAULT_BUN_VERSION = process.versions.bun ?? "latest"

Copy link
Copy Markdown
Owner Author

@SunsetTechuila SunsetTechuila Mar 23, 2025

Choose a reason for hiding this comment

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

process.versions.bun will always be undefended as this code will never run in bun. this is a file for other runtimes, bun.mjs is for bun:

"exports": {
".": {
"bun": "./dist/bun.mjs",
"types": "./dist/generic.d.ts",
"default": "./dist/generic.js"
},
"./package.json": "./package.json"
},

Copy link
Copy Markdown

@JounQin JounQin Mar 23, 2025

Choose a reason for hiding this comment

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

What is the difference between them then? If process.versions.bun is used, is there any other difference?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

there is:

is-bun-module/src/bun.ts

Lines 11 to 17 in eca11a6

const bunModules = { ...bundledBunModules } as Modules;
// don't do the same with node modules because `builtinModules` in bun contains even not implemented node modules
for (const moduleName of builtinModules) {
if (moduleName.startsWith("bun:")) {
bunModules[moduleName] ??= `>=${currentBunVersion}`;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I still don't get, can't we just write if (process.versions.bun) for this part and unify the two entries?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

unify the two entries?

this is not possible because I want this package to be runtime-independent and dynamically add missing bun modules to the list

dynamically add missing bun modules to the list

for this I need to use node:module, which I cannot dynamically require in CJS only with language features

Comment thread src/bun.ts Outdated
Comment thread src/shared.ts
export { default as implementedNodeModules } from "@assets/implemented-node-modules.json";

type SemVerBaseStringified = `${bigint}.${bigint}.${bigint}`;
type SemVerStringifiedWithReleaseName = `${SemVerBaseStringified}-${string}`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If the version pattern strictly follows the type, would something like the following help without depending on semver:

https://github.com/un-ts/synckit/blob/38578a5e6858d54958d41a17e3dc240a56da65d4/src/index.ts#L87-L106

Copy link
Copy Markdown
Owner Author

@SunsetTechuila SunsetTechuila Mar 23, 2025

Choose a reason for hiding this comment

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

having semver as a dependency allows me to easily and confidently write version range expressions of any complexity

removing it offers minimal benefits since it's typically already installed locally and adds less than 20KB to the minified bundle size

@SunsetTechuila SunsetTechuila merged commit 447fe2e into master Mar 23, 2025
5 of 6 checks passed
@SunsetTechuila SunsetTechuila deleted the update branch March 23, 2025 22:21
@github-actions
Copy link
Copy Markdown

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants