vercel / next.js

The React Framework
https://nextjs.org
MIT License
127.27k stars 27.02k forks source link

When building next js app, class names are minified which breaks libraries which depend on class names. (typegoose, webpack) #59594

Open tar-aldev opened 11 months ago

tar-aldev commented 11 months ago

Link to the code that reproduces this issue

https://github.com/tar-aldev/broken-class-names

To Reproduce

First serve it in dev mode to see how it should work

  1. copy .env.example and set credentials (or leave them as is)
  2. run mongodb via docker docker-compose up
  3. run next.js app via npm run nx serve next-broken
  4. IN browser see how it shows proper model name BrokenModel

Now build app and run it in prod mode

  1. build the app with npm run nx build next-broken
  2. serve it with npm run nx run next-broken:serve:production
  3. open browser or check the terminal of the IDE
  4. model name becomes p while it should still be BrokenModel

Current vs. Expected behavior

I'd expect model names (based on class name) to not be minified as it breaks typegoose. It worked fine in next.js 13 ("next": "13.4.19")

Verify canary release

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.0.0: Fri Sep 15 14:41:43 PDT 2023; root:xnu-10002.1.13~1/RELEASE_ARM64_T6000
Binaries:
  Node: 18.18.2
  npm: 9.8.1
  Yarn: N/A
  pnpm: 8.9.2
Relevant Packages:
  next: 14.0.5-canary.10
  eslint-config-next: 13.4.4
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.2.2
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Not sure

Additional context

It breaks since next 14 If I disable minification in next.conif.js

webpack: (config) => {
    config.optimization.minimize = false;
    return config;
  },

Then it works, but the build is not optimized which is not acceptable for prod.

As a workaround I could use TerserPlugin and set keep_classnames: true and then it works fine. (see with-terser` branch in github repo. But it would be nice to get an official guideline.

therk commented 11 months ago

Having the same issue even with the Next 13.5.6. It looks like it was raised earlier by @octet-stream via https://github.com/vercel/next.js/issues/58126 but closed as it didn't conform to the bug template.

octet-stream commented 11 months ago

Yes, this is because I forgot to change repository's visibility, so bot wasn't able to confirm reproduction URL and issue got closed. Also, I think this is where the issue occurred first: https://github.com/vercel/next.js/commit/30da48fcd28b1fc07dcfa06d89b28bec95897d10 They enabled minification for server side code, which is now handled by SWC afaik and it seem to me that SWC breaks class names for decorated classes.

So, the current solution would be just to disable minification for server code, like so (in next.config.js):

/** @type {import('next').NextConfig} */
module.exports = {
  experimental: {
    serverMinification: false,
  }
}
meronogbai commented 10 months ago

Setting serverMinification to false worked for me too. Thanks!

logusgraphics commented 9 months ago

This is also the case for me. I'm using sequelize-typescript with decorators and model injection depends on consistent class names. I disabled minification via:

const nextConfig = {
   swcMinify: false,
   ...
};

And works but I got this message:

Disabling SWC Minifer will not be an option in the next major version. Please report any issues you may be experiencing to https://github.com/vercel/next.js/issues
adamjakab commented 9 months ago

Similar scenario using Nextjs and Mikro-orm. Running in development mode all is good. When I export and run the standalone build, database table names are minified, hence breaking the functionality. A findAll() on a Log entity returns this and Postgres complains:
'select "g0".* from "g" as "g0" - relation "g" does not exist'. I tried first with swcMinify: false in my next.config.js and it did not change anything. Using @octet-stream's suggestion and setting serverMinification: false in the experimental block works and solves the issue.

So, not a big price to pay to have it working... I however very much dislike using anything called experimental in production.

mschipperheyn commented 9 months ago

Could we have a bit more granularity here? Bailing out of all server minification is not exactly an ideal solution if you want to have a compact docker container

octet-stream commented 9 months ago

I think another solution for this would be using EntitySchema syntax, instead of classes with decorators. Here's example in my repro repository: https://github.com/octet-stream/next-mikro-orm-minify-issue/tree/bdd4fe78d1247f209b69a9dd49cb6f71291e5efa/src/server/db/entities

More information can be found in the docs: https://mikro-orm.io/docs/defining-entities

This solution does not involve switching off minification for server code.

octet-stream commented 9 months ago

Can anybody confirm this issue exists on v14.1.0? Because I can't reproduce it in my repo anymore. They still minify the names, but now they're unique, so it still may occur at some point, if modules not bundled into a single file, I think.

You can try and build my repository from this branch: https://github.com/octet-stream/next-mikro-orm-minify-issue/tree/14.1.0

octet-stream commented 9 months ago

This of course is still an issue if Mikro ORM will try and query something form DB, because it gets a table name from Entity.name property. But this one can be fixes without disabling server minification via tableName param of the Entity decorator, like this:

@Entity({tableName: "my_entity"})
export class MyEntity { }

I have to say that this is not just Next.js issue, other meta frameworks also have similar problems: They either bundle everything into a single file (like Qwik, for example, because they use Vite as their bundler) and you get an error if you have any duplicate names across your project (like if you have an entity named User and valibot type with the same name), because bundler will rename identifiers in attempt to avoid duplicate names within single module scope, or they might not configure their minification to keep class names by default.

samcx commented 8 months ago

@tar-aldev We will need a smaller :repro: to investigate this properly (e.g., not including Nx, etc.).

octet-stream commented 8 months ago

You can take a look at mine: https://github.com/octet-stream/next-mikro-orm-minify-issue

samcx commented 8 months ago

@octet-stream I'm unable to build your :repro: for I think a different related issue →

   ▲ Next.js 14.2.0-canary.20
   - Environments: .env

   Creating an optimized production build ...
 ✓ Compiled successfully
 ✓ Linting and checking validity of types
   Collecting page data  ..[
  [class m],
  [class v],
  [class b extends _],
  [class _ extends v],
  [class g extends _]
]
DriverException: connect ECONNREFUSED 127.0.0.1:3306
octet-stream commented 8 months ago

Ah, I forgot to mention you'll need MySQL for this to get the right errors. Simple docker container should work.

I have updated instructions and the code. Run git pull origin main --rebase, then docker compose up -d and create env file: echo "DB_PORT=3308" > .env.local. This will create a container with MySQL, then you'll good to go. Create schema and run build command as the instruction says.

Sorry for this inconvenience.

angelxmoreno commented 8 months ago

I can confirm this is also an issue on v14 with TypeOrm. serverMinification: false works but instead, i went to my entity decorators and manually set the table names. Not minifying feels icky.

christhegrand commented 7 months ago

I can confirm this is also an issue on v14 with TypeOrm. serverMinification: false works but instead, i went to my entity decorators and manually set the table names. Not minifying feels icky.

Can you say more about what you changed? My entities are defined like this:

@Entity({name: 'organizations'})

But I'm still seeing the minification issue here.

angelxmoreno commented 7 months ago

my suggestion is to first try serverMinification: false and see if it builds. if it does not address those issues first.

christhegrand commented 7 months ago

Yes, I did that and it fixed the runtime errors I was seeing. What are the table name changes you made that fixed the problem without having to disable the server-side minification?

angelxmoreno commented 7 months ago

well i did a few things,

  1. all my @Entity() were changed to @Entity({name: 'table_name'})
  2. manually imported my entities into my datasource instead of using paths.
samcx commented 7 months ago

@octet-stream is this supposed to be capital -D?

CleanShot 2024-04-10 at 13 11 31@2x

octet-stream commented 7 months ago

No. But you can omit this flag, it just detaches container's output from your terminal, so you can continue use it in the same window.

DavidChouinard commented 7 months ago

Can confirm this also causes issues with TypeORM. We kept getting the following error message: CircularRelationsError: Circular relations detected: Dependency Cycle Found: s -> s. To resolve this issue you need to set nullable: true somewhere in this dependency structure.

Setting serverMinification: false resolves the issue.

angelxmoreno commented 7 months ago

@DavidChouinard also, circular relations could be solved by using the Relation generic: https://typeorm.io/#relations-in-esm-projects

markedwards commented 6 months ago

@angelxmoreno You can still get the dependency cycle error with TypeORM even with the Relation generic, if the minified class names end up in a certain way.

The only true solution at this point seems to be using EntitySchema.

Here's an issue in the TypeORM repo that's been open for years.

angelxmoreno commented 6 months ago

@angelxmoreno You can still get the dependency cycle error with TypeORM even with the Relation generic, if the minified class names end up in a certain way.

The only true solution at this point seems to be using EntitySchema.

Here's an issue in the TypeORM repo that's been open for years.

guess you are right. I've never come across the situations posted in the issue but that's more of how dependencies are created. For many of those one-off situations, the limitation is JS not TypeOrm. But I digress.

Gjum commented 5 months ago

Copypasting my solution here for visibility.

Looping over getMetadataArgsStorage() and running Object.defineProperty() for each registered entity solved this for me. This works with serverMinification enabled and does not rely on EntitySchema. See https://github.com/typeorm/typeorm/issues/4714#issuecomment-1557229315 for the original idea.

@Entity("my_table")
export class MyTable {
  // ...
}
import { getMetadataArgsStorage } from "typeorm"

// this reads @Entity("my_table") or @Entity({ name: "my_table" })
for (const table of getMetadataArgsStorage().tables) {
  Object.defineProperty(table.target, "name", { value: table.name })
}

const db = new DataSource({ entities: [MyTable], ...options })
AleFons commented 5 months ago

Copypasting my solution here for visibility.

Looping over getMetadataArgsStorage() and running Object.defineProperty() for each registered entity solved this for me. This works with serverMinification enabled and does not rely on EntitySchema. See typeorm/typeorm#4714 (comment) for the original idea.

@Entity("my_table")
export class MyTable {
  // ...
}
import { getMetadataArgsStorage } from "typeorm"

// this reads @Entity("my_table") or @Entity({ name: "my_table" })
for (const table of getMetadataArgsStorage().tables) {
  Object.defineProperty(table.target, "name", { value: table.name })
}

const db = new DataSource({ entities: [MyTable], ...options })

I have spent the last couple of months going insane over this, I've looked absolutely everywhere, I've tried disabling minification, tried to keep classnames the same, I've swapped my entire app to use entityschema and nothing worked.

This seems to have solved it. I have no words.

pascuflow commented 3 months ago

Has anyone gotten MikroORM 6.3 working in production with server minification using Nextjs 14.2?

None of these worked: