hardyscc / nestjs-dynamoose

Dynamoose module for Nest
MIT License
142 stars 24 forks source link

Breaking change #837

Closed onurkose closed 1 year ago

onurkose commented 1 year ago

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

feat: move tableName from model to options

This is a breaking change and requires proper versioning.

Expected behavior

I was running into a simple problem with aws serverless and needed to dump node_modules and hit npm install afterwards. Because of the config change, all my table names defaulted to model names. When the models start running on production it re-created all tables because the table names mismatched.

Minimal reproduction of the problem with instructions

What is the motivation / use case for changing the behavior?

Environment


NestJS version: X.Y.Z
NestJS Dynamoose version: X.Y.Z
Dynamoose version: X.Y.Z


For Tooling issues:
- Node version: XX  
- Platform:  

Others:

onurkose commented 1 year ago

After further investigation, even after upgrading the config to the new version, table name settings were not able to read from config service:

import { DynamooseModule } from 'nestjs-dynamoose';
import { UserSchema } from './user.schema';
import { UserService } from './user.service';

@Module({
  imports: [
    ConfigModule.forRoot({ isGlobal: true }),
    DynamooseModule.forFeatureAsync([
      {
        name: 'User',
        useFactory: (_, configService: ConfigService) => {
          return {
            schema: UserSchema,
            options: {
              tableName: configService.get<string>('USER_TABLE_NAME'),  <-- This won't work anymore, table name defaults to User automatically.
            },
          };
        },
        inject: [ConfigService],
      },
    ]),
  ],
  providers: [
    UserService,
    ...
  ],
})
export class UserModule {}
hardyscc commented 1 year ago

@onurkose i just tested without any problem, are u using dynamoose version 3.2.0?

onurkose commented 1 year ago

yes, it's dynamoose 3.2.0 I just downgraded nestjs-dynamoose to 0.5.3 and it works just fine.

hardyscc commented 1 year ago

@onurkose I tested using aws-nestjs-starter without any problem, since it won't use the dynamoose auto table create feature, if you are using dynamoose auto table create feature, please report to dynamoose about this bug.

famence commented 1 year ago

https://github.com/hardyscc/nestjs-dynamoose/commit/15a06f2cce1ab4024c89a92661eb82bed69b5c97 should only affect a model name, and not a table name.

It would be great if you fork aws-nestjs-starter and reproduce your problem so that i can debug it and contribute a hot fix

hardyscc commented 1 year ago

@onurkose I also tested the dynamoose auto create table feature using options.tableName, it also works correctly.

ianzone commented 1 year ago

I confirm this issue. The "tableName" doesn't work anymore. It created a "tenants" table based on the "name". image

ianzone commented 1 year ago

and this works. image

ianzone commented 1 year ago

dude, you really need to do the semantic versioning and change the major version when involves breaking changes.

hardyscc commented 1 year ago

@ianzone I will try my best to keep semantic versioning...