Make it possible to export container dependant variables #562

hexa00 · 2017-09-25T15:28:38Z

This patch introduces the initialize function inside a module, this
function is called with the root container of the application at
application startup.

This enables the logger module to export a logger object fetched via
container.get(ILogger) such that this new logger object can be
exported to code that does not use injection.

Signed-off-by: Antoine Tremblay antoine.tremblay@ericsson.com

hexa00 · 2017-09-25T15:29:05Z

See: #517

akosyakov · 2017-09-26T05:57:47Z

I don't think we should generalize it, a case with the root logger is special. It would be nice to keep the module interface simple. Also the first thing to access from the root container must be an application, otherwise the root container is not considered complete, meaning that other extensions could override some services require for logging after loading the loader module.

Is there a reason not to do in onActivation of an application?
Like here: https://github.com/inversify/InversifyJS/blob/master/wiki/activation_handler.md#activation-handler

svenefftinge · 2017-09-26T06:32:27Z

I agree with Anton, I think it should just be a side-effect in container's load.

hexa00 · 2017-09-26T10:43:07Z

Ha yes indeed onActivation is what I need, thanks!

hexa00 · 2017-09-26T17:01:21Z

The latest change is WIP , I don't understand how to make this global var work, ideas ?
Note the terminal-process changes are just there for testing
the logger is undefined when trying to log the terminal creation
You can test by starting a term

svenefftinge · 2017-09-26T18:15:36Z

onActivation is called on creation of a logger. If it is not demanded through @Inject it is not created. I think @akosyakov meant to use onActivation of FrontendApplication resp. BackendApplication bindings.

hexa00 · 2017-09-26T18:23:09Z

@svenefftinge but if I do that the application will need to depend on the logger it works for core but it would not work for another package.

Also independenly of that do you have an idea for the gobal export ?

hexa00 · 2017-09-26T18:24:00Z

Note we will want do that with the localize package also...

svenefftinge · 2017-09-26T18:30:30Z

I think you would need to rebind the FrontendApplication within the logger container module and add an onActivation hook. But I'm not so happy with that, too. Could you outline why we need that for the localization? Maybe with two cases, it is worth to introduce a callback as you did in the first PR. :)

hexa00 · 2017-09-26T18:35:14Z

In the case of the localization we want to make it easy to localize a string anywhere in theia.

So you could call localize("mystring"); directly from anywhere without injecting the MessageTranslation class (localize is exported from that class).

But we need the MessageTranslation class in the inversify context so that it can access the user preferences to select the proper language.

So we need to bridge the gap between the inversify context and static context same as with the logger.

hexa00 · 2017-09-26T18:38:40Z

Actually thinking about this now localize can't depend on preferences since preferences depend on too much stuff... I'm not sure how to structure this properly yet it seems :(

hexa00 · 2017-09-26T18:45:03Z

Actually for localization I think we could have a mock preferences class until we get the real preferences and then rebind those.

So it would work properly once you get the whole application but otherwise use a failover.

svenefftinge · 2017-09-26T18:45:54Z

I thought we were doing this not for convenience, but because we need to log in a static context?

svenefftinge · 2017-09-26T18:47:08Z

adding injection for logger and localize should be the default. For localize we should try to avoid doing this global scope hack.

hexa00 · 2017-09-26T18:48:30Z

For localize it would mean basically every class would have a localize injected given that you would want to localize log messages too possibly.

That's what I'm trying to avoid.

hexa00 · 2017-09-26T18:50:12Z

Note the logger is pretty much as oblivious
So I was tempted to use that as convinence for the root logger too rather than use injection

svenefftinge · 2017-09-26T19:24:34Z

I think we should only localize user-facing messages. logs are for developers.

hexa00 · 2017-09-26T19:32:01Z

Maybe that restrains the places where it's used enought...

akosyakov · 2017-09-27T06:37:29Z

Why should it be global, is not possible to use the module import?

akosyakov · 2017-09-27T10:30:57Z

BTW what about an idea to rebind console functions instead of exposing our logger?

hexa00 · 2017-09-27T10:32:37Z

For localize it has to be gobal to do like this:
https://github.com/lmcbout/theia/blob/2b05667dbcc565c42f2f71d583fe4c4173eb405f/packages/workspace/src/browser/workspace-frontend-contribution.ts

See:

export namespace WorkspaceCommands {
    export const OPEN: Command = {
        id: 'workspace:open',
        label: localize("Open", "Open...")
    };
}

hexa00 · 2017-09-27T10:34:36Z

For the logger there is less reason to be global at the moment since I don't have a static context to call it from like I had before.

But I wanted to do the logger as an example for localize to follow and provide a general way to do this when need be for anything.

svenefftinge · 2017-09-27T11:02:46Z

Ok, do we need localize in the container, then?

svenefftinge · 2017-09-27T11:04:04Z

I guess you want to use server-side preferences for the locale?

hexa00 · 2017-09-27T11:09:32Z

Ok, do we need localize in the container, then?

Not sure what you mean ?

I guess you want to use server-side preferences for the locale?

Yes that's the plan and this is another issue since core has strings to localize but if localize depends on preferences and preferences depend on core/filesystem/workspace it's a problem... but let's tackle that later.

For now I'm trying to figureout out how an extension can contribute a global to be able to call something in an inversify context from static context.

akosyakov · 2017-09-27T11:13:01Z

For now I'm trying to figureout out how an extension can contribute a global to be able to call something in an inversify context from static context.

Why should it be global? Globals are generally discouraged to avoid namespace conflicts. Why not use the module import as you already do for localize, e.g.:

import { logger } from '@theia/core/common/logger';

logger.log('lalala');

akosyakov · 2017-09-27T12:58:07Z

or import * as log from '@theia/core/lib/common/logger'

hexa00 · 2017-09-27T13:00:56Z

Actually the problem was that doing

import { logger } from '@theia/core/lib/common';

Gives:

var common_1 = require("@theia/core/lib/common");
common_1.logger.info("Starting terminal process: " + options.command + ","

And here logger is undefined

However doing:

import { logger } from '@theia/core/lib/common/logger';

Gives:

var logger_1 = require("@theia/core/lib/common/logger");
logger_1.logger.info("Starting terminal process: " + options.command + ","

And this is OK.

So the reexporting in the index is the issue, not sure why ... ideas?

akosyakov · 2017-09-27T13:07:43Z

With CommonJS the exports is done by value rather than by reference, hence the issue I have.

because of it again? basically, reexport creates a new module and reassigns properties from reexported modules, it happens only once

akosyakov · 2017-09-27T13:12:13Z

What if you just reexport functions, like info, error and so on? e.g.

import * as logger from '@theia/core/lib/common/logger';
logger.info('blabla');

or

import { info } from '@theia/core';
info('blabla');

What with the idea to rebind console.log to logger.info?

hexa00 · 2017-09-27T13:20:59Z

So looking at index.js:

function __export(m) {
    for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./types"));
`

So yes this makes an new export object so here it's taken by value and can't be changed.

So I'm just going to directly import it for this case and use:

import { logger } from '@theia/core/lib/common/logger';
logger.info('foo');

hexa00 · 2017-09-27T14:27:37Z

@svenefftinge

think you would need to rebind the FrontendApplication within the logger container module and add an onActivation hook. But I'm not so happy with that, too. Could you outline why we need that for the localization? Maybe with two cases, it is worth to introduce a callback as you did in the first PR. :)

Are you ok with adding this, given the issue with the localization I exposed ?

svenefftinge · 2017-09-27T17:15:05Z

It somehow feels like we are going down the wrong route here, but I don't see alternatives :-|
For localization, I am not sure if we really need to do that with backend preferences. Maybe local storage would work well, too.

svenefftinge · 2017-09-27T17:15:42Z

But if you want to move on with the logger, I'm fine with adding the rebind for now.

hexa00 · 2017-09-27T18:23:01Z

It somehow feels like we are going down the wrong route here, but I don't see alternatives :-|

I agree, however at lest the initialze thing will be quite self contained and generic.

For localization, I am not sure if we really need to do that with backend preferences. Maybe local storage would work well, too.

Yes I tought about that it's just that, I wonder if preferences could have a local storage backend ? so we could use the same API if the filesystem is not present?

But if you want to move on with the logger, I'm fine with adding the rebind for now.

Thanks I'll repost the original PR

hexa00 · 2017-09-27T19:47:49Z

@svenefftinge back to to originial proposal... is it OK ?

svenefftinge · 2017-09-27T20:42:56Z

It is ok, although if we go with the generic callback variant I would prefer using DI and our bindContributionProvider. I.e. define an interface like Initializer and register it using bindContributionProvider. Then, the first thing you do with a container is to obtain all instances and call their initialize method.

hexa00 · 2017-09-27T23:01:10Z

That sounds like a good idea, I'll give it a try.

This patch introduces the initialize method in Frontend/BackendApplicationContribution this is called as the first thing the application does before onStart or configure. This enables the logger module to export a logger object fetched via container.get<ILogger>(ILogger) such that this new logger object can be exported to code that does not use injection. Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>

hexa00 · 2017-10-02T13:09:07Z

@svenefftinge reworked that with bindContributionProvider... looks better ?

hexa00 · 2017-10-04T12:31:31Z

@svenefftinge I'd like to merge this to enable localization to go forward and use this too.. any objections?

akosyakov · 2017-10-04T12:36:38Z

Could we reuse BackendApplicationContribution.configure for it? And add FrontendApplicationContribution.configure in the frontend?

hexa00 · 2017-10-04T12:40:24Z

@akosyakov no since we can't be certain of the order of the contribution... if another contribution is loaded before and expects things to be initialzed it will fail :(

Unless there's a way to put some priority there.. but I'd like to avoid depending on the order the modules are loaded.

akosyakov · 2017-10-04T13:15:22Z

no since we can't be certain of the order of the contribution... if another contribution is loaded before and expects things to be initialzed it will fail :(

In which cases? Do you mean other contributions want to use logger? I thought they should inject an instance of Logger?

Or add initialize callback to BackendApplicationContribution and FrontendApplicationContribution which is called before configure? I thought these interfaces were to provide application lifecycle callbacks.

hexa00 · 2017-10-04T13:55:01Z

In which cases? Do you mean other contributions want to use logger? I thought they should inject an instance of Logger?

Yes but if we give the option that they don't have to inject this, it has to work in all cases.

True I could add initialize to the Backend/FrontendApplicationContribution I wonder why I did not think of this before now hehe.

I'll refactor as such.

hexa00 · 2017-10-04T15:14:07Z

@akosyakov OK with recent changes ?

akosyakov · 2017-10-04T15:17:10Z

packages/core/src/node/backend-application.ts

+
+        for (const contribution of this.contributionsProvider.getContributions()) {
+            if (contribution.initialize) {
+                contribution.initialize();


could be done separately, but we should also wrap calls in try-catch in the backend

I'm adding this to this PR after all.. since it's quite trivial

akosyakov

LGTM

Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>

hexa00 requested review from akosyakov and svenefftinge September 25, 2017 15:29

hexa00 force-pushed the logger-context branch from c720ad4 to da55bfc Compare September 25, 2017 16:49

hexa00 force-pushed the logger-context branch from da55bfc to d214726 Compare September 26, 2017 16:58

hexa00 force-pushed the logger-context branch from d214726 to c720ad4 Compare September 27, 2017 19:46

hexa00 force-pushed the logger-context branch from c720ad4 to ee96767 Compare September 29, 2017 17:25

hexa00 force-pushed the logger-context branch 2 times, most recently from c102fbc to dacdd68 Compare October 4, 2017 15:13

akosyakov reviewed Oct 4, 2017

View reviewed changes

akosyakov approved these changes Oct 4, 2017

View reviewed changes

Add try/catch to contribution calls in backend-application

a992732

Signed-off-by: Antoine Tremblay <antoine.tremblay@ericsson.com>

hexa00 merged commit f383ce3 into eclipse-theia:master Oct 4, 2017

hexa00 deleted the logger-context branch October 4, 2017 16:02

hexa00 mentioned this pull request Oct 4, 2017

Make the logger accessible outside the inversify context #517

Closed

Make it possible to export container dependant variables #562

Make it possible to export container dependant variables #562

Conversation

hexa00 commented Sep 25, 2017

hexa00 commented Sep 25, 2017

akosyakov commented Sep 26, 2017

svenefftinge commented Sep 26, 2017 • edited Loading

hexa00 commented Sep 26, 2017

hexa00 commented Sep 26, 2017

svenefftinge commented Sep 26, 2017

hexa00 commented Sep 26, 2017

hexa00 commented Sep 26, 2017

svenefftinge commented Sep 26, 2017

hexa00 commented Sep 26, 2017

hexa00 commented Sep 26, 2017

hexa00 commented Sep 26, 2017 • edited Loading

svenefftinge commented Sep 26, 2017 • edited Loading

svenefftinge commented Sep 26, 2017

hexa00 commented Sep 26, 2017

hexa00 commented Sep 26, 2017 • edited Loading

svenefftinge commented Sep 26, 2017

hexa00 commented Sep 26, 2017

akosyakov commented Sep 27, 2017

akosyakov commented Sep 27, 2017

hexa00 commented Sep 27, 2017

hexa00 commented Sep 27, 2017

svenefftinge commented Sep 27, 2017

svenefftinge commented Sep 27, 2017

hexa00 commented Sep 27, 2017 • edited Loading

akosyakov commented Sep 27, 2017

akosyakov commented Sep 27, 2017

hexa00 commented Sep 27, 2017

akosyakov commented Sep 27, 2017 • edited Loading

akosyakov commented Sep 27, 2017 • edited Loading

hexa00 commented Sep 27, 2017

hexa00 commented Sep 27, 2017

svenefftinge commented Sep 27, 2017

svenefftinge commented Sep 27, 2017

hexa00 commented Sep 27, 2017 • edited Loading

hexa00 commented Sep 27, 2017

svenefftinge commented Sep 27, 2017

hexa00 commented Sep 27, 2017

hexa00 commented Oct 2, 2017

hexa00 commented Oct 4, 2017

akosyakov commented Oct 4, 2017 • edited Loading

hexa00 commented Oct 4, 2017

akosyakov commented Oct 4, 2017

hexa00 commented Oct 4, 2017

hexa00 commented Oct 4, 2017

akosyakov Oct 4, 2017

Choose a reason for hiding this comment

hexa00 Oct 4, 2017

Choose a reason for hiding this comment

akosyakov left a comment

Choose a reason for hiding this comment

svenefftinge commented Sep 26, 2017 •

edited

Loading

hexa00 commented Sep 26, 2017 •

edited

Loading

svenefftinge commented Sep 26, 2017 •

edited

Loading

hexa00 commented Sep 26, 2017 •

edited

Loading

hexa00 commented Sep 27, 2017 •

edited

Loading

akosyakov commented Sep 27, 2017 •

edited

Loading

akosyakov commented Sep 27, 2017 •

edited

Loading

hexa00 commented Sep 27, 2017 •

edited

Loading

akosyakov commented Oct 4, 2017 •

edited

Loading