spike: using diary.js as a hard dependency #24

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@iammerrick
Contributor

No description provided.

@iammerrick
Contributor

Attention @IgorMinar

@iammerrick iammerrick added cla: yes and removed cla: no labels Mar 8, 2014
@IgorMinar IgorMinar commented on the diff Mar 10, 2014
src/injector.js
+Diary.reporter(new ConsoleReporter({
+ console: console
@IgorMinar
IgorMinar Mar 10, 2014 Member

this is not right. di should not configure reporters because then everybody using di will be stuck with this reporter config.

reporter config is a responsibility of the environment in which the instrumented code runs.

@vojtajina vojtajina commented on the diff Mar 12, 2014
karma.conf.js
@@ -14,13 +14,15 @@ module.exports = function(config) {
{pattern: 'example/testing/*.js', included: false},
{pattern: 'node_modules/es6-shim/es6-shim.js', included: false},
{pattern: 'node_modules/q/q.js', included: false},
+ {pattern: 'node_modules/Diary.js/src/**/*.js', included: false},
@vojtajina
vojtajina Mar 12, 2014 Contributor

You import the module as "diary" (in package.json), here you are referencing it as "Diary.js", it's weird that it works, you should call it the same thing everywhere. I vote for "diary", but it's up to you.

@iammerrick
iammerrick Mar 12, 2014 Contributor

The reason for the fragmentation is because we didn't have the diary namespace on npm. Now that we do this fragmentation doesn't have to exists.

@vojtajina vojtajina commented on the diff Mar 12, 2014
src/injector.js
@@ -4,9 +4,17 @@ import {getUniqueId} from './profiler';
// NOTE(vojta): should we rather use custom lightweight promise-like wrapper?
import {resolve} from 'q';
+import {Diary} from 'Diary/diary';
@vojtajina
vojtajina Mar 12, 2014 Contributor

Now you call it "Diary", all these should be same IMHO. I even don't understand how come that this works ;-)

@vojtajina vojtajina commented on the diff Mar 12, 2014
test-main.js
@@ -18,7 +18,8 @@ require.config({
paths: {
'assert': './node_modules/pipe/node_modules/assert/dist/amd/assert',
- 'q': './node_modules/q/q'
+ 'q': './node_modules/q/q',
+ 'Diary': './node_modules/Diary.js/src'
@vojtajina
vojtajina Mar 12, 2014 Contributor

Oh, this is why it works... ;-)

@vojtajina
vojtajina Mar 12, 2014 Contributor

Please make all of these the same ("Diary", "diary", "Diary.js")

@iammerrick
iammerrick Mar 12, 2014 Contributor

Yeah, it's an npm ownership problem which his now been resolved.

@iammerrick
iammerrick Mar 12, 2014 Contributor

Something a bit odd is the diary vs Diary in that classes are conventionally title cased and modules are conventionally lowercased. Note that q is actually installed Q in the absence of a module system. How would you feel about a import Diary from diary.

@vojtajina
vojtajina Mar 12, 2014 Contributor

Sure, that's ok, if you wanna use the default export.

@vojtajina vojtajina commented on the diff Mar 12, 2014
karma.conf.js
{pattern: 'node_modules/pipe/node_modules/assert/dist/amd/**/*.js', included: false}
],
preprocessors: {
'src/**/*.js': ['traceur'],
'test/**/*.js': ['traceur'],
- 'example/**/*.js': ['traceur']
+ 'example/**/*.js': ['traceur'],
+ 'node_modules/Diary.js/src/**/*.js': ['traceur']
@vojtajina
vojtajina Mar 12, 2014 Contributor

I would suggest, not preprocess the code and rather use already transpiled code.

@iammerrick
iammerrick Mar 12, 2014 Contributor

How would that work with the traceur run time stuff?

@vojtajina
vojtajina Mar 12, 2014 Contributor

Exactly how you do this on production: The NPM package that di depends on has to contain transpiled code (es5 code).

@iammerrick
iammerrick Mar 12, 2014 Contributor

Help me understand, if I compile to ES5 code using traceur then the runtime needs to have the traceur runtime installed ahead of time correct? I could bundle the runtime with diary.js but then we are shipping it twice in the case of di.js. Am I missing something?

@vojtajina
vojtajina Mar 12, 2014 Contributor

No, you are not missing anything.

@vojtajina
Contributor

Sorry for the delay @iammerrick . I left some comments.

@iammerrick iammerrick closed this Mar 25, 2014
@iammerrick iammerrick deleted the iammerrick:feature/logging-diary.js branch Mar 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment