Skip to content

Use multiple exports #38

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

Closed
ibc wants to merge 5 commits into from
Closed

Use multiple exports #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c587ac
Use multiple exports (attempt 2)
ibc Mar 13, 2025
8da13d1
done
ibc Mar 13, 2025
1576c20
test
ibc Mar 13, 2025
dd7610a
done?
ibc Mar 15, 2025
871a3a2
tsconfig.json: Use lib es2024
ibc Mar 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ npm install rtp.js
- All RTP and RTCP classes, types and packet related helpers are exported by the `packets` module.

```ts
import { packets } from 'rtp.js';
import * as packets from 'rtp.js/packets';

const {
isRtp,
Expand Down Expand Up @@ -46,15 +46,16 @@ npm install rtp.js
- The `utils` module exports some generic helpers and utilities.

```ts
import { utils } from 'rtp.js';
import * as utils from 'rtp.js/packets';

const view = utils.stringToDataView('foo');
const view = utils.stringToDataView('fooœæ€ñ#¢∞Ω©bar');
```

- CommonJS is also supported:

```ts
const { packets, utils } = require('rtp.js');
const packets = require('rtp.js/packets');
const utils = require('rtp.js/utils');
```

## Authors
Expand Down
4 changes: 1 addition & 3 deletions npm-scripts.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,14 @@ async function run() {
}

case 'typescript:build': {
installDeps();
buildTypescript({ force: true });

break;
}

case 'typescript:watch': {
deleteLib();
executeCmd('tsc --noEmit --watch');
executeCmd('rollup --config --watch');

break;
}
Expand All @@ -82,7 +81,6 @@ async function run() {
}

case 'test': {
buildTypescript({ force: false });
test();

break;
Expand Down
33 changes: 25 additions & 8 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 13 additions & 12 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,19 @@
"type": "git",
"url": "git+https://github.com/versatica/rtp.js.git"
},
"main": "lib/rtp.js.bundle.cjs",
"module": "lib/rtp.js.bundle.mjs",
"types": "lib/rtp.js.bundle.d.ts",
"exports": {
"import": "./lib/rtp.js.bundle.mjs",
"require": "./lib/rtp.js.bundle.cjs"
"./packets": {
"typedoc": "./src/packets/public.ts",
"types": "./lib/packets.d.ts",
"import": "./lib/packets.mjs",
"require": "./lib/packets.cjs"
},
"./utils": {
"typedoc": "./src/utils/public.ts",
"types": "./lib/utils.d.ts",
"import": "./lib/utils.mjs",
"require": "./lib/utils.cjs"
}
Comment on lines +16 to +27
Copy link

@satoren satoren Mar 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

First, I will comment based on the assumption that the purpose of splitting is to reduce the bundle size.

I think it’s a matter of preference, but I believe having a main that exports everything would be a good idea. Since consumers usually use a bundler, when using ESM, tree shaking will still be performed even without excessive splitting. Additionally, adding "sideEffects": false to package.json is expected to further improve tree shaking efficiency.

People who care about size are definitely using a bundler.

Copy link
Member Author

@ibc ibc Mar 16, 2025

Choose a reason for hiding this comment

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

I don't care about library size. This is intended to be a library for Node despite it works in browsers too. My goal is to provide a modern import mechanism instead of the classic index based single import which IMHO looks ancient.

With that in mind, I am not sure why I am doing bundling of packets and utils. Instead I would prefer to have 3 lib folders, lib-es, lib-cjs and types, and replicate in all those folders the whole file tree of src folder:

  • lib-es/ would contain .mts files.
  • lib-cjs/ would contain .cjs files.
  • types/ would contain .d.ts files.
  • I see no reason to have two types/ folder, one with .d.mts files and another with .d.ts files, because in my experiments, those files are exactly the same when generating them with rollup with rollup-dst-plugin using format "es" and "cjs".

Then the exports in package.json would be similar to what I have done in this PR, so entry points would also be "rtp.js/packets" and "rtp.js/utils".

In summary, despite what I have done in this PR so far, I see no reason to bundle anything.

Does it make sense?

Copy link
Member Author

@ibc ibc Mar 16, 2025

Choose a reason for hiding this comment

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

Actually I'm doing exactly that with babel directly and getting rid of rollup. Working on it.

Copy link

@satoren satoren Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see. I don’t think exporting individually is necessarily the modern approach.

Having multiple entry points for exports has specific advantages, and if none of these apply, there is no real benefit to splitting:

  • In the case of CJS, tree shaking does not work properly, so splitting helps reduce bundle size (improving runtime load speed).
  • For large libraries, splitting can improve load speed during development.
  • When a library serves a wide range of purposes and has diverse use cases.

Additionally, even in a Node.js environment, keeping the size small can be beneficial, such as for AWS Lambda, etc.

Copy link
Member Author

@ibc ibc Mar 17, 2025

Choose a reason for hiding this comment

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

Honestly I am not focusing on tree shaking or lib size. My points are:

  • I don't want to only have CJS in the published library. It's 2025 already.
  • Having only ESM in published library is a bit problematic yet due to some projects/frameworks/utils not supporting or enabling ESM yet.
  • So I want to expose both ESM and CJS in the published library.
  • And I also want to expose multiple entry points because we are in 2025 and we can do it better than exposing everything in a single index/dist/bundle main and unique export.
  • But that doesn't mean that we need to bundle anything and that's why I am getting rid of rollup. It's up to applications and library consumers to decide whether they want to bundle their code (and deps) or not. A library should not make those assumptions.

Copy link
Member Author

@ibc ibc Mar 17, 2025

Choose a reason for hiding this comment

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

@satoren I'm doing all that in this new PR #39.

},
"files": [
"npm-scripts.mjs",
Expand Down Expand Up @@ -49,21 +56,15 @@
"jest": {
"verbose": true,
"testEnvironment": "node",
"moduleFileExtensions": [
"js",
"mjs",
"ts",
"mts"
],
"testRegex": "src/test/.*.test.ts",
"coveragePathIgnorePatterns": [
"src/Logger.ts",
"src/utils",
"src/test"
],
"cacheDirectory": ".cache/jest"
},
"dependencies": {
"@types/node": "^22.13.10",
"debug": "^4.4.0",
"supports-color": "^10.0.0"
},
Expand Down
46 changes: 32 additions & 14 deletions rollup.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,59 @@
import esbuild from 'rollup-plugin-esbuild';
import dts from 'rollup-plugin-dts';

// Make it explicit that we don't want to include external deps in bundle
// files.
const external = id => !/^[./]/.test(id);
// Make it explicit that we don't want to include external dependencies in
// bundle files.
const external = ['debug'];

export default [
/**
* Create ES and CJS bundle files and their source map files.
* Create ES and CJS bundles of packets and utils and also source map
* files.
*
* NOTE: We also need to bundle utils/helpers.ts because it's used by
* packets and, if not specified, rollup will generate two helpers-xxx.js
* files, ES and CJS, both with .js extension, and will make Node believe
* that both are CJS due to their .js extension.
*/
{
input: 'src/index.ts',
plugins: [esbuild({ include: /\.ts$/ })],
input: {
packets: 'src/packets/public.ts',
utils: 'src/utils/public.ts',
'utils/helpers': 'src/utils/helpers.ts',
},
plugins: [esbuild()],
output: [
{
format: 'es',
file: 'lib/rtp.js.bundle.mjs',
dir: 'lib',
entryFileNames: '[name].mjs',
sourcemap: true,
},
{
format: 'cjs',
file: 'lib/rtp.js.bundle.cjs',
dir: 'lib',
entryFileNames: '[name].cjs',
sourcemap: true,
},
],
external: external,
},
/**
* Create a .d.ts bundle file for TypeScript types.
* Create ES and CJS types bundles of packets and utils.
*/
{
input: 'src/index.ts',
plugins: [dts({ include: /\.ts$/ })],
output: {
format: 'es',
file: 'lib/rtp.js.bundle.d.ts',
input: {
packets: 'src/packets/public.ts',
utils: 'src/utils/public.ts',
},
plugins: [dts()],
output: [
{
format: 'es',
dir: 'lib',
entryFileNames: '[name].d.ts',
},
],
external: external,
},
];
2 changes: 0 additions & 2 deletions src/index.ts
View file
Open in desktop

This file was deleted.

137 changes: 0 additions & 137 deletions src/packets.ts
View file
Open in desktop

This file was deleted.

Loading