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

The code published to npm is minified #4065

Open
Raynos opened this issue Jul 9, 2023 · 12 comments
Open

The code published to npm is minified #4065

Raynos opened this issue Jul 9, 2023 · 12 comments
Labels

Comments

@Raynos
Copy link

Raynos commented Jul 9, 2023

Since the code in the npm tarball is minified by default ( https://socket.dev/npm/package/preact/files/10.15.1/dist/preact.mjs )

It's nearly impossible to debug or profile the preact framework when using it to build my app.

Please do not publish minified code and let me run my own minifier.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Jul 9, 2023

The main goal of us doing so has been to

  • ensure the lowest size, we know what properties are safe to mangle
  • consistent performance, no minifier/transpiler would insert a snippet that performs poorly

I do see what you are saying about traces becoming unreadable while investigating bugs/performance. It's been one of my main motivators to fight for "readable" dist bundles as well.

However doing so would most likely warrant a major as we do ship properties that we need mangled for our plugin ecosystem, the reconciler is pluggable so devtools/signals/fast-refresh rely on that being consistent.

If you are facing a bug and don't know how to reproduce due to the bundle being unreadable I am open to figure this out together on a GitHub discussion/...

@Raynos
Copy link
Author

Raynos commented Jul 9, 2023

For my personal use case I am likely to copy paste the source code of preact into my application and use it directly so that I can have the unminified version.

I ran into the same issue with react, and was fighting with nextjs in that it does not allow me to set NODE_ENV

It would help me if you committed a version of preact.single-file.js into the git repo so I could copy a single file instead of the entire source code.

The way i used to copy jquery.js into my vendor/jquery.js codebase and also copy vendor/jquery.min.js and control what I load when and where for development / staging / production.

@Raynos
Copy link
Author

Raynos commented Jul 9, 2023

I guess having dist/preact.raw.js next to dist/preact.min.js would also help me, in that I can quickly and easily copy the raw single file source code or import it with require('preact/dist/preact.raw.js')

This would be a non breaking change, just an extra file into the npm publish tarball.

@rschristian
Copy link
Member

For my personal use case I am likely to copy paste the source code of preact into my application and use it directly so that I can have the unminified version.

As Jovi mentioned, this could be an issue as the plugin system relies upon mangled properties for hooking into the reconciler. If you're only using Preact core, you shouldn't run into any issues, but you are locking yourself off from devtools/prefresh/etc.

Unfortunately I'm not sure a mangled, but unminified, distribution file would end up being much more readable... _children still needs to become __k, etc.

@Raynos
Copy link
Author

Raynos commented Jul 10, 2023

I appreciate the technical explanation of why this wouldn't work with the ecosystem and the devtools.

I am however completely blown away that this technical limitation exists. it boggles my mind.

A mangled & unminified build would not really be helpful. I guess all the plugin ecosystem tools would have to support both mangled & unmangled copy of preact with some kind of config setting.

@rschristian
Copy link
Member

rschristian commented Jul 11, 2023

Well, I'm not quite sure what other alternatives exist. The problem is that properties need to remain consistent across versions and across every distribution method. A few CDNs mangle themselves (whether they should is another issue) and bundlers may need some config to ensure both Preact & the add-on have consistently mangled properties.

Mangling ahead of time certainly isn't ideal, but I don't know of any better options that doesn't offload the problem onto end users.

@Raynos
Copy link
Author

Raynos commented Jul 11, 2023

The consistent mangling for ecosystem I can understand.

Would be nice if the ecosystem respects both the unmangled and mangled property names, so I could use either the min or raw version.

@MicahZoltu
Copy link
Contributor

I'm also frustrated by this right now as I have some code that is getting called too frequently and what is happening inside preact is quite opaque. The included TypeScript definition files help a lot with reading the code, but unfortunately Firefox (at least) doesn't unmangle variables in the debugger so this is what I see:
image

My position on this is that minification/mangling isn't worth the costs, especially given that basically every file served over the internet is gzipped. A given word/token (e.g., _children) is essentially one lookup table entry in a gzipped file plus maybe a byte or two of data per reference, thus mangling gives very minimal benefits. Meanwhile, the cost is that it takes significantly longer to debug applications than if the code was un-mangled.

I would encourage comparing just gzipping vs gzipping+mangling to make sure you are actually benefiting in a significant way at least. My guess is that mangling isn't actually saving much. Even if it is, there should still be a way to run without mangling for people who don't want/need mangling.

@dliebner
Copy link

Is there a way to import the source version?

I'm currently using:
import * as preactSignals from '@preact/signals-core';

I tried this:
import * as preactSignals from '@preact/signals-core/src/index.ts';
... but rollup complained about it not being JavaScript even though I have the typescript plugin loaded, and other typescript imports in my bundle are working fine.

@rschristian
Copy link
Member

Is there a way to import the source version?

As mentioned, no.

rollup complained about it not being JavaScript even though I have the typescript plugin loaded, and other typescript imports in my bundle are working fine.

You'd need to configure your TS plugin to compile node_modules, that's not standard behavior.

@matthewmueller
Copy link

I was able to get a PoC working with esbuild by:

Tweaking preact/package.json:

  1. Add preact/src as a valid export
"./src": {
  "types": "./src/index.d.ts",
  "browser": "./src/index.js",
  "import": "./src/index.js"
},
  1. Tweak preact/jsx-dev-runtime to point to src:
"./jsx-dev-runtime": {
  "types": "./jsx-runtime/src/index.d.ts",
  "browser": "./jsx-runtime/src/index.js",
  "import": "./jsx-runtime/src/index.js"
},

Currently jsx-dev-runtime points to the same thing as jsx-runtime, so unless that's for historical reasons, might make sense to tweak for dev. Otherwise an ./jsx-runtime/src export could be added.

Then you adjust the alias settings in esbuild for development:

Alias: map[string]string{
	"preact":             "preact/src",
	"preact/jsx-runtime": "preact/jsx-dev-runtime",
},

I'd be open to submitting a PR if this is something the Preact team would consider!

@johrstrom
Copy link

johrstrom commented Jul 9, 2024

  1. Add preact/src as a valid export

I'd be open to submitting a PR if this is something the Preact team would consider!

I would +1 this as somewhere in between 10.20.1 and 10.22.1 an exported function started being compiled as $2 that's conflicts with jQuery/Ruby on Rails javascript.

I'm currently exploring something like this in esbuild to force it to resolve to the src instead of the dist directories.

const preactSrcResolvePlugin = {
  name: 'preactSrcResolve',
  setup(build) {
    build.onResolve({ filter: /preact/ }, args => {
      const basePath = `${__dirname}/node_modules/preact`;
      if (args.path == 'preact') {
        return { path: `${basePath}/src/index.js` }
      } else if(args.path == 'preact/hooks') {
        return { path: `${basePath}/hooks/src/index.js` }
      }
    })
  },
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants