Remove type: "module" to fix yarn2 issue

[JonasKruckenberg]

yarn2 was mistakenly trying to load the import files even when required from a nodejs context. Removing "type": "module" fixes this.

What kind of change does this PR introduce? (check at least one)

• [x] Bugfix
• [ ] Feature
• [ ] Docs
• [ ] New Binding Issue #___
• [ ] Code style update
• [ ] Refactor
• [ ] Build-related changes
• [ ] Other, please describe:

Does this PR introduce a breaking change? (check one)

• [ ] Yes. Issue #___
• [x] No

The PR fulfills these requirements:

• [ ] When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
• [ ] A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

• [ ] A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

[amrbashir]

I don't want to do this for two reasons.

1. I want to push the ecosystem to use ESM instead of CJS, and with Vite and some other big projects already doing so, I think Tauri should too.
2. I had similar issue with my project vite-plugin-tauri, it didn't matter which node version I use. However using yarn3, it is fixed.

My two cents is if it works on yarn3 then we should just tell users to upgrade to yarn3

[FabianLars]

1: i don't really see how that's related, esm keeps working (i think because of the exports block in package.json?). But i'm pretty sure i'm missing something here or just don't understand what you mean. 2: that's valid 🤷

But anyway, i don't really care, i just approved to say that it doesn't (seem to) break anything.

[amrbashir]

My point is, If we remove "type": "module", then we are not actively encouraging(forcing?) users to switch to ESM themselves, instead we are helping people stay on CJS which I don't want to do but I am not speaking for everyone on the team and definitely don't know what is best for the Tauri ecosystem.

[FabianLars]

I'm totally there with you. But as far as i understand it, it doesn't make a difference here. type: module specifies how .js files should be interpreted, but we overwrite that with the conditional exports in the exports block anyway.

But yeah i guess i'd prefer this more explicit way (==specifying the type) too, especially if a yarn version update fixes this 🤷 for that matter, the node docs suggest to always specify it too, even if you use CommonJS.

[JonasKruckenberg]

My two cents is if it works on yarn3 then we should just tell users to upgrade to yarn3

Sorry my bad, when I said yarn2 I meant >=yarn2, so the post-berry versions.

And while yes, getting everybody to use is a good goal, nodejs is not quite there yet imo. Yes they "support" esm now, but it's terribly hacky and works for almost no projects (As it forces you to use one or the other, without real interop)

But either way, we can remove type: "module" or the require exports. Because both together confuses the heck out of node

[amrbashir]

Does npm and pnpm work fine with type: module and only yarn2 and yarn3 have a problem or all of them don't work ?

[JonasKruckenberg]

Does npm and pnpm work fine with type: module and only yarn2 and yarn3 have a problem or all of them don't work ?

I will do some testing and report 👍🏻

[lucasfernog]

Note that we're currently facing an issue with the Rollup build on #3244 apparently because @rollup/plugin-commonjs doesn't ship with a "type": "module" package.json (@rollup/plugin-node-resolve does). We can't just test NPM vs Yarn1 vs Yarn 2 vs PNPM, we also need to check the module bundle tools.

[amrbashir]

Why do we even need these two rollup plugins for the api package? I think we can replace rollup with tsup for the api package.

[JonasKruckenberg]

Why do we even need these two rollup plugins for the api package? I think we can replace rollup with tsup for the api package.

Tsup can't build minified browser builds, that's what those plugins are for.

[amrbashir]

Why do we even need these two rollup plugins for the api package?

This is a genuine question btw, I don't understand what are their usage in the api package because if I understand correctly those are used to pull code from node_modules into the final bundle but the api package doesn't depend on any npm package and probably shouldn't in the future.

I think we can replace rollup with tsup for the api package.

Tsup can't build minified browser builds, that's what those plugins are for.

Last time I checked it has a minify option and iife target.

[JonasKruckenberg]

Last time I checked it has a minify option and iife target.

Can you maybe screenshot where it says that? Their docs site is still doesn't support mobile

[FabianLars]

Last time I checked it has a minify option and iife target.

Yeah, but you can't have non-minified and minified output at the same time, and you also can't give the minified file a special name. Unless i missed something ofc, but we had a similar problem in tauri-plugin-sql and switched back to rollup because of this

[amrbashir]

Last time I checked it has a minify option and iife target.

Can you maybe screenshot where it says that? Their docs site is still doesn't support mobile

It has another docs website, https://tsup.egoist.sh/#bundle-targets and https://tsup.egoist.sh/#minify-output

Yeah, but you can't have non-minified and minified output at the same time, and you also can't give the minified file a special name. Unless i missed something ofc, but we had a similar problem in tauri-plugin-sql and switched back to rollup because of this

We can have two npm scripts to fix this but I really don't see why would we want non-minified version.

[FabianLars]

but I really don't see why would we want non-minified version.

I thought we wouldn't minify the esm output, which apparently we do. But isn't that something we better leave to the user's bundler? (quick google search suggests the same)

[amrbashir]

Unless there is a good reason I think we should minify the esm output to reduce the package size. After all we advocate for smaller size apps.

[JonasKruckenberg]

Unless there is a good reason I think we should minify the esm output to reduce the package size. After all we advocate for smaller size apps.

Generally no one minifies the esm/cjs-builds (so builds for bundlers) as it makes errors/crashes very obtuse without a sourcemap. At which point you might as well just bundle it unminified, sourcemaps are humongous.

[FabianLars]

Unless there is a good reason I think we should minify the esm output

https://stackoverflow.com/questions/48673408/should-javascript-npm-packages-be-minified Can provide more links later, kinda busy lazy rn.

Edit: So tldr, with minifying our esm output we probably even increase the app size in the end, because we make it harder for bundlers to be 100% efficient. Plus (like Jonas said), it's absolute garbage for debugging (especially if the user doesn't use typescript)

Generally no one minifies the esm

Yeah, looking through my node-modules folder right now, it looks like we're indeed the only ones doing it lol

[JonasKruckenberg]

Whatever we end up doing, we should ensure to have unified tsconfigs, build configs, cargofmt configs between the api package and all our plugins. I noticed that the configurations between those vary wildly and especially the api package is very different to the plugins.

To that end, IMO we don't even need an IIFE build, since tauri supports esm on all versions (right?) so we can just provide esm builds across the board, one minified (for no bundler usage) and one uniminfied for bundler usage

[amrbashir]

Generally no one minifies the esm/cjs-builds (so builds for bundlers) as it makes errors/crashes very obtuse without a sourcemap. At which point you might as well just bundle it unminified, sourcemaps are humongous.

TIL

Anyways, providing a minified iife(umd) and unminified esm/cjs with tsup should be a trivial task.

To that end, IMO we don't even need an IIFE build, since tauri supports esm on all versions (right?) 

We need the iife(umd) version for withGlobalTauri option.

[JonasKruckenberg]

Anyways, providing a minified iife(umd) and unminified esm/cjs with tsup should be a trivial task.

I'm open to switching the plugins to tsup once I understand that tool a bit better, but I think we can't change the api package. It requires us to produce chunks for all the "submodules" so import window from "@tauri-apps/api/window" works. I don't think tsup can do this 🤔 correct me if I'm wrong.

But it's also not that big of a deal, I had a look at the rollup.config.js file and we can simplify it a lot! Should be way more maintainable then.

We need the iife(umd) version for withGlobalTauri option.

Oh true, that makes sense.

[amrbashir]

I'm open to switching the plugins to tsup once I understand that tool a bit better, but I think we can't change the api package. It requires us to produce chunks for all the "submodules" so import window from "@tauri-apps/api/window" works. I don't think tsup can do this 🤔 correct me if I'm wrong.

Currently we compile each file under src using rollup. The same can be done with tsup.

[JonasKruckenberg]

Currently we compile each file under src using rollup. The same can be done with tsup.

I know, but that's actually pretty suboptimal, we should use manualChunks with rollup too

Edit: I'll have a look at what we can do with tsup

[amrbashir]

I actually don't want to use tsup right now, it was just a mere observation that the api package is so simple and don't need all of these rollup plugins and we don't even need babel.

In the light of that, I don't think we should block this PR on some rollup plugins not playing nicely with "type": " module".

Edit: I'll have a look at what we can do with tsup

I am a bit ahead of you. I had some time to kill on my phone so I will push a branch with the changes so you can see the tradeoffs if there is any.

[FabianLars]

How you can do all that stuff on your phone is beyond me, really (so props for that). I'm not even able to properly comment on issues on my phone lol.

Edit: Sorry for off-topic

[JonasKruckenberg]

We absolutely don't need babel and like 60% of those plugins! See, here I'm a bit ahead of you 😉 bc I have a local branch with way simpler rollup config

And without stupid babel

[amrbashir]

How you can do all that stuff on your phone is beyond me, really (so props for that). I'm not even able to properly comment on issues on my phone lol.

Edit: Sorry for off-topic

I rarely code on my phone but when I do I have termux (Android terminal) and neovim including tsserver and rust-analyzer for autocompletion.

We absolutely don't need babel and like 60% of those plugins! See, here I'm a bit ahead of you 😉 bc I have a local branch with way simpler rollup config

And without stupid babel

cool, here is the tsup branch so you can test which is better

I accidentally removed some eslint deps (which is kinda useless too) so don't look much into it.

[JonasKruckenberg]

Actually @amrbashir do you think we still need a cjs build of the api? It's a browser only library, it won't even work without the tauri context around it. I just did some research and I guess 95% of all build tools can deal with esm by default and browserify as esmify.

So should we drop support? 🤔

[amrbashir]

I don't remember why we decided to keep shipping cjs but I think webpack4 (create-react-app) didn't handle it well.

[nothingismagick]

Is there a problem with leaving it in? because if 5% can't use what we offer then I feel like we are doing a disservice.

[lucasfernog]

Last time I checked one of the major bundlers (Webpack 4, Rollup, Vite, i don't remember exactly) was forcedly trying to read CJS files and I expect some issues if we drop support, so this is definitely a no we can't.

[amrbashir]

yeah just checked our discord chat, it was definitely webpack4 (used in create-react-app).

[JonasKruckenberg]

yeah just checked our discord chat, it was definitely webpack4 (used in create-react-app).

Okay yeah makes sense (even though I hate it 😄) so we leave it.

Anyway, here are some results of comparing rollup (without babel) to tsup:

Generated Files

rollup

tsup

The way tsup generates files when code-splitting is enabled differs dramatically to that of rollup. Tsup generates shared chunks and produces "frontend" files for the named chunks that just delegate to to the shared chunks. This causes a lot of files to be generated.

Config

rollup

import fg from 'fast-glob'
import { basename } from 'path'
import typescript from '@rollup/plugin-typescript'
import { terser } from 'rollup-plugin-terser'
import pkg from './package.json'

const manualChunks = Object.fromEntries(
  fg.sync('./src/*.ts').map((p) => [basename(p, '.ts'), [p]])
)

export default [
  {
    input: 'src/index.ts',
    manualChunks,
    external: [
      ...Object.keys(pkg.dependencies || {}),
      ...Object.keys(pkg.peerDependencies || {})
    ],
    output: [
      {
        format: 'esm',
        entryFileNames: '[name].mjs',
        chunkFileNames: '[name].mjs',
        dir: './dist'
      },
      {
        format: 'cjs',
        entryFileNames: '[name].cjs',
        chunkFileNames: '[name].cjs',
        dir: './dist'
      }
    ],
    plugins: [typescript()]
  },
  {
    input: 'src/index.ts',
    output: {
      format: 'iife',
      name: '__TAURI__',
      file: '../../core/tauri/scripts/bundle.js',
    },
    plugins: [
      typescript(),
      terser()
    ]
  }
]

tsup

import { defineConfig } from 'tsup'

export default defineConfig(() => [
  {
    entry: ['src/*.ts'],
    outDir: 'dist',
    format: ['esm', 'cjs'],
    clean: true,
    splitting: true,
    dts: true,
    sourcemap: false,
    keepNames: true,
  },
  {
    entry: { bundle: 'src/index.ts' },
    outDir: '../../core/tauri/scripts',
    format: ['iife'],
    globalName: '__TAURI__',
    splitting: false,
    clean: false,
    dts: false,
    sourcemap: false,
  }
])

As you can see even the cleaned-up rollup config is significantly more verbose than the tsconfig.

Tsup code also generates a lot of

__name(<function here>, "<function name here>");

annotations and I have no idea what they do. I'm not even sure if this is good or bad so little do I get what its for 😂 .

Edit: It's caused by the keepNames option, so I guess it's for debuggability of minified builds? Why is this option a thing?

[JonasKruckenberg]

The sheer number of generated files with tsup has me a bit worried, so as-is I would prefer to keep rollup (with the simplified config) but maybe tsup can be configured differently. I definitely like the simple configuration

[amrbashir]

yeah tsup keepNames can be omitted, I was just testing what it does and it isn't worth it.

I like that you even improved my tsup config, I didn't know you can return two objects.

Anyways, splitting can be turned off for tsup which will reduce the number of generated files drastically.

The thing I am most afraid about is that tsup generates chunks for .d.ts and it might mess up IDE go to definition DX, but if it does, we can use tsc for generating declarations directly instead of tsup.

[JonasKruckenberg]

I like that you even improved my tsup config, I didn't now you can return two objects.

I know that trick from vite 😉

Anyways, splitting can be turned off for tsup which will reduce the number of generated files drastically.

The thing I am most afraid about is that tsup generates chunks for .d.ts and it might mess up IDE go to definition DX, but if does we can use tsc for generating declarations directly instead of tsup.

Well if we can get rid of those weird chunks to make the output cleaner I'm all in with tsup 👍🏻

[amrbashir]

Yeah either way is good but I feel like we drifted a bit from the main goal of this PR which is to fix yarn2 weirdness with type: module

I am still kinda opposed to removing it because yarn2 only had issue. if it works with npm, pnpm and yarn1 and yarn3 correctly then we should just tell users to upgrade from yarn2 to yarn3 (which thry should be doing anyways).

[JonasKruckenberg]

Yeah either way is good but I feel like we drifted a bit from the main goal of this PR which is to fix yarn2 weirdness with type: module

I am still kinda opposed to removing it because yarn2 only had issue. if it works with npm, pnpm and yarn1 and yarn3 correctly then we should just tell users to upgrade from yarn2 to yarn3 (which thry should be doing anyways).

I think I meant yarn2+ originally, but honestly f** yarn2+ 😂 (I know telling users this ain't gonna work but still)

[amrbashir]

Ah sorry, this the second time I miss tgat.

Still, I don't think removing it is the right way forwards and if yarn2+ have issues with it, maybe it is worth to file a bug report.

Do you by chance have a reproducible example?

[JonasKruckenberg]

Do you by chance have a reproducible example?

Not reproducible bc of the whole js git dependency thingy. But I just spun up the same basic test project using npm, pnpm, yarn1, yarn2+ and yarn2+pnp and I couldn't reproduce this problem. Maybe we can close this PR.

[amrbashir]

alright, we can discuss this again if it comes up after the release