Build hybrid package - provide commonjs and ESM entry points

[susnux]

Provide also an entry point for packages using ESM instead of commonjs.
The target for ESM is set to ES2020 / ES2022 which is fully supported by node 16, the lowest node version nextcloud supports.

[ChristophWurst]

👍

[skjnldsv]

Is this actually needed?

This library is never used as is, but always in another app.
The app should be responsible for babel and transpiling into a specific ES level.

All the other libs are just exporting straight away without any transformations beside es6/commonjs exports syntax

[ChristophWurst]

Really? I thought we did run also the libs through babel+browserslist. Otherwise users of the lib also have to transpile dependencies, no?

[ChristophWurst]

As far as I understand this aspect hasn't changed. It just moved from one configuration file to another.

[susnux]

@skjnldsv this is only copied from js to json to be independent to node resolution (mjs vs cjs). So this is untouched, but from my point of view I agree that this is not needed, see my comment below #535 (comment)

[skjnldsv]

Otherwise users of the lib also have to transpile dependencies, no?

Which we do everywhere already. Transpiling twice is not necessary.
Lots of our libs do not pass anything through babel (despite having an unnecessary babel.config.js present)

• https://github.com/nextcloud/nextcloud-cypress/blob/master/rollup.config.mjs
• https://github.com/nextcloud/nextcloud-files/blob/master/rollup.config.js
• https://github.com/nextcloud/nextcloud-upload/blob/master/rollup.config.js
• https://github.com/nextcloud/nextcloud-auth/blob/master/rollup.config.js
• https://github.com/nextcloud/nextcloud-event-bus/blob/master/rollup.config.js
• https://github.com/nextcloud/nextcloud-axios/blob/master/rollup.config.js
• https://github.com/nextcloud/nextcloud-initial-state/blob/master/rollup.config.js

Some are doing it though:

• https://github.com/nextcloud/nextcloud-dialogs/blob/master/rollup.config.mjs
• https://github.com/nextcloud/nextcloud-logger/blob/master/package.json
• https://github.com/nextcloud/nextcloud-l10n/blob/master/package.json
• https://github.com/nextcloud/nextcloud-browser-storage/blob/master/package.json
• https://github.com/nextcloud/nextcloud-sharing/blob/main/package.json
• https://github.com/nextcloud/nextcloud-router/blob/master/package.json
• https://github.com/nextcloud/nextcloud-capabilities/blob/master/package.json
• https://github.com/nextcloud/nextcloud-paths/blob/master/package.json

[skjnldsv]

It seems the one you created indeed use babel.
My guess is that you went for the same pattern on their initialisation?

Anyway, do you have any clue about one library that would be used as is without going through webpack in one of our apps?

[ChristophWurst]

https://www.npmjs.com/package/@nextcloud/vue

[skjnldsv]

@nextcloud/vue is babelizing the dist output :)

[susnux]

Anyway, do you have any clue about one library that would be used as is without going through webpack in one of our apps?

I am not sure all apps transpile the dependencies, while this should be done by any application targeting browsers instead of node, as you can not trust every package matches your browser targets.
Because most webpack configs look like this:

// Rules
{
    test: /\.js$/,
    loader: 'babel-loader',
    exclude: /node_modules/,
}

So all dependencies are excluded from the babel transpilation.

But I do not suggest enabling transpilation of all dependencies using babel-loader, the build times are awful.
Instead I recommend to get rid of babel (we do not need ES5 at all, as all supported browsers support ES6), and instead use the build in webpack loader for JS files and only transpile all output on the minification step by using the ESBuildMinifyPlugin (together with browserslist-to-esbuild for setting the output target).
Besides that all dependencies are transpiled this also reduces the bundle size and build time significantly.

[skjnldsv]

Interesting, this is a quite wide discussion.
I'm not confortable having only the three of us take a decision that have a company-wide impact.

Can you raise this in https://github.com/nextcloud/standards ?
So we can discuss and decide what to do consistently for all libs?

[susnux]

Another question which came up while writhing the PR is:

Is the babel transpiling needed at all? Is there any specific reason for doing it (same with the core-js dependency)?
Because the javascript, emitted by tsc, is already fully compatible with all of the browsers nextcloud supports (checked that with check-es-compat), e.g. this could look like this: 907bbee

[skjnldsv]

Another question which came up while writhing the PR is:

Is the babel transpiling needed at all? Is there any specific reason for doing it (same with the core-js dependency)? Because the javascript, emitted by tsc, is already fully compatible with all of the browsers nextcloud supports (checked that with check-es-compat), e.g. this could look like this: 907bbee

That was my point :)
We don't need transpiling on libs that are imported by apps.
Apps are already transpiling for our browserlist config

[susnux]

That was my point :) We don't need transpiling on libs that are imported by apps. Apps are already transpiling for our browserlist config

So should I include the commit for dropping babel transpilation with this PR or open a new one after this is merged?

[skjnldsv]

Approving as this does not change the actual behaviour.
Regardless of the discussion it raised :)