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

fix: Publish config uses ES dist file #429

Closed
wants to merge 1 commit into from

Conversation

Chocobozzz
Copy link
Contributor

@Chocobozzz Chocobozzz commented Oct 29, 2024

Use ES dist file as default export so consumer don't have to rebuild all files

Especially useful for consumers to not polyfill node dependencies of bittorrent-tracker dependency

@mrlika
Copy link
Member

mrlika commented Oct 29, 2024

When publishing an npm package, the best practice is to export transpiled .js source files.

The package's consumers might have specific build processes or configurations. Providing the source files allows them to integrate a package seamlessly into their workflow regarding compatibility, flexibility, tree-shaking, etc.

For example, a node dependency polyfill may already be included in a codebase, and including it the second time makes the end application much bigger.

Can the bundled ES module be somehow exported as an alternative to the existing one? https://webpack.js.org/guides/package-exports/

@mrlika
Copy link
Member

mrlika commented Oct 29, 2024

Something like:

 "exports": {
    ".": "./lib/index.js",
    "./esm": "./dist/p2p-media-loader-core.es.js"
  }

But this approach will also require somehow binding types to the "./esm" export.

Use ES dist file as default export so consumer don't have to rebuild all
files

Especially useful for consumers to not polyfill node dependencies
of bittorrent-tracker dependency
@Chocobozzz
Copy link
Contributor Author

I understand your point. However front-end libraries tend to ship the ES dist file by default (shaka player, hls.js or video.js do this kind of thing). It's also the recommended solution chosen by Vite: https://vite.dev/guide/build#library-mode

I suggest to use the exports method so consumers have the choice to use the source file, while specifying the ES module by default:

  • Most devs don't want to spend time on mocking bittorrent-tracker server dependencies
  • Your build process is good enough to tree-shake what p2p-media-loader doesn't need (especially the bittorrent-tracker server)
  • Using a custom exports requires consumers to have specific tsconfig settings (moduleResolution/module options) that may not be compatible with their current setup
  • Some frameworks (like Angular) don't allow to easily change the build setup to choose the appropriate file
  • Devs that want build control should have enough knowledge to choose the appropriate exports

I updated the PR with my proposal.

@mrlika
Copy link
Member

mrlika commented Nov 3, 2024

Thanks, we will test the proposed approach and give our feedback

@mrlika
Copy link
Member

mrlika commented Nov 16, 2024

I researched and didn't see that modern NPM packages that export ES modules do bundling.

But bundling dependencies is even considered harmful. The NPM ecosystem expects packages to manage dependencies via package.json. This allows for better dependency resolution and version management and ensures that packages aren't included multiple times in the end application bundle.

ChatGPT

Conclusion
Exporting the transpiled JavaScript code without bundling dependencies and polyfills is the recommended approach. It adheres to NPM best practices, keeps your package lightweight, and avoids potential conflicts and issues for the end-user.

https://stackoverflow.com/questions/71275009/bundling-and-publishing-an-npm-library-is-it-common-to-resolve-all-dependencies

According to me, you should never bundle your package since it makes tree shaking more complicated for the consumer. This applies to esm packages

https://www.reddit.com/r/typescript/comments/dsidr4/best_practices_for_organizingarchitecting_a_npm/

don't provide bundles: the consuming applications are already responsible for bundling/transpiling/minifying/polyfilling. consumers want to be in control of these things

@mrlika
Copy link
Member

mrlika commented Nov 18, 2024

@Chocobozzz, we solved your issue using conditional exports. Could you verify the solution on your end?

"exports": {
  ".": {
    "p2pml:core-bundle": "./dist/p2p-media-loader-core.es.js",
    "import": "./lib/index.js"
  }
},
"types": "./lib/index.d.ts",

A bundler or JS runtime should specify the p2pml:core-bundle condition to use the bundled version of the P2P Media Loader Core module.

Vite:

export default defineConfig({
  plugins: [
    //nodePolyfills(),
    react(),
  ],
  resolve: {
    conditions: ['p2pml:core-bundle'],
  },
});

Node.js, webpack, Rollup: https://webpack.js.org/guides/package-exports/#conditions-custom

@mrlika
Copy link
Member

mrlika commented Nov 21, 2024

Hi @Chocobozzz, do you have updates on the approach described in the comment above? If there are no issues, we want to include it in the new release.

@Chocobozzz
Copy link
Contributor Author

Chocobozzz commented Nov 21, 2024

Hi and sorry for the late answer. Conditional export is interesting but unfortunately the angular framework doesn't allow us to add conditions. If we could use Vite directly in Angular I would not have raised this issue ^^ I consider this is a specific PeerTube use case and I don't expect you to fix it. I'll just create another NPM package of p2p-media-loader in @peertube scope to export the bundle by default. In the meantime I hope webtorrent/bittorrent-tracker#535 will fix the root cause of the issue (ie importing server node dependencies).

@Chocobozzz Chocobozzz closed this Nov 21, 2024
@mrlika
Copy link
Member

mrlika commented Nov 21, 2024

@Chocobozzz, I may be wrong, but there is a way to override conditionNames in internal Angular's Webpack config via an Angular build plugin:

npm install @angular-builders/custom-webpack --save-dev

angular.json

{
  "projects": {
    "your-app-name": {
      "architect": {
        "build": {
          "builder": "@angular-builders/custom-webpack:browser", // Change this line
          "options": {
            "customWebpackConfig": {
              "path": "./webpack.partial.js", // Path to your partial config
              "mergeStrategies": {
                "resolve.conditionNames": "replace" // Specify merge strategy
              }
            },
            // ... other existing options
          },
          // ... other configurations
        },
        // ... other architectures
      }
    }
  }
}

// webpack.partial.js

module.exports = {
  resolve: {
    conditionNames: [...],
  },
};

@Chocobozzz
Copy link
Contributor Author

Chocobozzz commented Nov 21, 2024

Angular is moving on esbuild, and we use this new builder in peertube: https://angular.dev/tools/cli/build

But thanks, I discovered https://www.npmjs.com/package/@angular-builders/custom-esbuild#custom-esbuild-application. I'll try it

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