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

CommonJS support #294

Closed
alekitto opened this issue Jul 27, 2019 · 6 comments
Closed

CommonJS support #294

alekitto opened this issue Jul 27, 2019 · 6 comments

Comments

@alekitto
Copy link

Is this proposal usable in CommonJS environment?
As, if stated, decorators are not values shouldn't be possible to export/import a decorator via require.

If this is correct, how the decorators in the current proposal could be used in node.js? ES modules support is still experimental and cross-compatibility between ES modules and CJS modules is not complete.

Node frameworks should be rewritten to support current decorators?

@dead-claudia
Copy link

As the proposal is currently written, it looks very much like a "no, it's not", and some of the semantics aren't really compatible with a value-oriented import anyways. You could do an inline hack with vm, but it'd be easier to just create a separate module, import it, and use the resulting value. And in any case, it's not synchronous.

@alekitto
Copy link
Author

I see. This however seems to me very limiting for testability and in case a dependency should be needed by a decorator.

For testability this proposal closes the possibility of unit testing a decorator. What can be done is just to apply a decorator to a class and test that class to produce the expected result, but the decorator itself is not really "unit" tested as should always be coupled to a class.

Additionally the dependency injection is very limited as any dependency should be available statically to the decorator. For example: @logged works great when used with console.log, but with a proper logger (that streams to a file, connects to a database or an API) not available globally or statically (not a singleton) cannot be used and a old-style call to the logger should be done in a getter/setter. Even in the case the logger is passed as argument to the @logger decorator, the end-user module should import the logger, adding a dependency to a concrete implementation.
This leads to have no way of mocking without a compiler/transpiler/recompiler and no external or dynamic configuration on which logger should be used.

Or am I completely wrong?

@dead-claudia
Copy link

@alekitto The logger could expose a hook that lets you manually set the logger function, as most libraries do already.

Additionally the dependency injection is very limited as any dependency should be available statically to the decorator.

The binding has to be available statically, but the value itself can be provided later. One could imagine an API like this:

let currentLogger = console

export function setLogger(logger) {
	currentLogger = logger
}

export decorator @logger(name) {
	@wrap(f => function () {
		currentLogger.log(name)
		return f.apply(this, arguments)
	})
}

The logger value obviously might not be the correct value immediately, but the binding is there and is readily accessible. This is all you really need for a logger supporting dependency injection.

@littledan
Copy link
Member

I agree with @isiahmeadows 's analysis. In this variant, decorators are not first-class JavaScript values, and cannot be used in CommonJS natively, unless a particular convention is used within transpilers and if they are partially compiled away.

@Jamesernator
Copy link

Jamesernator commented Sep 27, 2019

This leads to have no way of mocking without a compiler/transpiler/recompiler and no external or dynamic configuration on which logger should be used.

The binding has to be available statically, but the value itself can be provided later. One could imagine an API like this

You could also just load a different module in for the purposes of tests, hopefully existing test solutions will add module mocking soon after ESM in Node ships.

@littledan
Copy link
Member

The new decorator proposal, with decorators as ordinary JS values, means that we have CommonJS support :)

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

4 participants