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

Update documentation to warn that @Inject via constructor is unsupported on vite 5.0.11 and above #182

Closed
zacharyweidenbach opened this issue Oct 16, 2024 · 7 comments

Comments

@zacharyweidenbach
Copy link

zacharyweidenbach commented Oct 16, 2024

Description

Vite 5.0.11 and greater uses esbuild which cannot support the emitDecoratorMetadata in the tsconfig.json because this feature requires a type system which esbuild does not provide.

vitejs/vite#4884 (comment)
vitejs/vite#15565

The workaround might be to use a different transpiler than the official vite-plugin-react-swc but I have not confirmed. Regardless, this would be helpful to know beforehand in the documentation.

@zacharyweidenbach
Copy link
Author

I did notice this PR that has been in the works since September. #175

v3.0 looks like it will stop using the legacy decorators implementation, removing the need for the experimentalDecorators flag in the tsconfig.json file. Will this also fix the issue I have reported?

@guyca
Copy link
Collaborator

guyca commented Oct 17, 2024

Hey @zacharyweidenbach Indeed v3 will stop using the legacy decorators implementation. But in fact I don't think v2 actually needs emitDecoratorMetadata, it's experimentalDecorators that's actually required.

Few things to keep in mind:

  1. Obsidian works best with Babel, so you might want to use the standard Vite configuration. Example can be found here.
  2. Obsidian can still be used with vite-plugin-react-swc - it will just require a tiny bit more boilerplate. Example can be found here.

@guyca
Copy link
Collaborator

guyca commented Oct 17, 2024

@zacharyweidenbach Do you have experience writing an SWC plugin? I think adapting our existing Babel plugin to an SWC plugin similar to this should be fairly easy.

Edit: I usually miss notifications on GitHub by available on Discord

@zacharyweidenbach
Copy link
Author

zacharyweidenbach commented Oct 17, 2024

@guyca

I don't have much experience with writing SWC plugins. I can take a swing at it. Before I get too far though, I want to confirm that what I'm trying to do with class injection should work.

This is what I want to do but it seems fail to inject the dependency. At runtime, the CreateOneMethod dependency is undefined. This is true even if I use field injection instead of injecting via the constructor. If I refactor CreateOneMethod to a graph, and inject it into the TodoService, it does inject properly.

@Singleton()
@Graph()
export class ApplicationGraph extends ObjectGraph {
  @Provides()
  store(): Store {
    return getStore();
  }
}

@Inject(ApplicationGraph)
export class CreateOneMethod {
  constructor(@Inject() private store: Store) {
  }

  public execute(todoTitle: string, todoId: string) {
    this.store.save(todoTitle, todoId);
  }
}

@Singleton()
@Graph({ subgraphs: [ApplicationGraph] })
export class TodoService extends ObjectGraph {
  constructor(@Inject() private createOneMethod: CreateOneMethod) {
    super();
  }

  @Provides()
  createOne(): CreateOneMethod['execute'] {
    return this.createOneMethod.execute;
  }
}

@guyca
Copy link
Collaborator

guyca commented Oct 17, 2024

@zacharyweidenbach Are you using Obsidian's Babel plugin react-obsidian/dist/transformers/babel-plugin-obsidian ?

If not, then you'll have to adapt your code slightly:

@Singleton() @Graph()
export class ApplicationGraph extends ObjectGraph {
  @Provides({name: "store"})
  store(): Store {
    return getStore();
  }

  @Provides({name: "createOneMethod"}
  createOneMethod({store}) {
    return new CreateOneMethod(store);
  } 
}

@Singleton() @Graph({ subgraphs: [ApplicationGraph] })
export class TodoService extends ObjectGraph {
  @Provides({name: "createOne"})
  createOne({createOneMethod}): CreateOneMethod['execute'] {
    return this.createOneMethod.execute;
  }
}

Notice that each provider needs to explicitly specify a name and the the providers must use destructuring when accessing their dependencies. These "quirks" can be avoided by using the Babel plugin. In you case I can implement an SWC plugin.

@guyca
Copy link
Collaborator

guyca commented Oct 17, 2024

I'm not sure TodoService needs to be a Graph. Seems like it should be a regular class and be injected with its dependencies. Something like:

@Injectable(ApplicationGraph)
export class TodoService {
  @Inject("createOneMethod") createOneMethod: createOneMethod!;

  createOne(): CreateOneMethod['execute'] {
    return this.createOneMethod.execute;
  }
}

@zacharyweidenbach
Copy link
Author

Closing. After a conversation in discord. It was discovered that I was incorrectly using class injections with graphs. Class dependencies are not recursively resolved in the same way that graphs are. However, for vite, I did need to add a plugin that I did not see in the documentation in order to prevent compile time errors from being thrown due to field injection in a class, via @Inject private someField!: SomeType;

  plugins: [
    react({
      babel: {
        plugins: [
          'react-obsidian/dist/transformers/babel-plugin-obsidian',
          ['@babel/plugin-proposal-decorators', { legacy: true }],
          ['@babel/plugin-transform-class-properties', { loose: true }], // add this
        ],
      },
    }),
  ],

Thanks to guyca for being so responsive and helpful!

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

No branches or pull requests

2 participants