Port to TypeScript #71

tp · 2018-01-31T14:27:37Z

since you'd would accept a TypeScript port, I have started one. At the moment it just compiles, but it's not final.

I try to keep the changes (for the user) as simple as possible, but it should probably be released as 2.0 if merged (at least the TypeScript using users will get some errors due to fixes – for JS users I strive for full compatibility).

For now I have 2 questions:

You advertise including the script via https://cdn.rawgit.com/jonnyreeves/js-logger/master/src/logger.min.js
I would like to use the src folder exclusively for the TypeScript sources, and put the transpiled files in lib (as is common convention). That would obviously break the above file. I hope no-one is using that in production anyway, but if you want to keep that stable we would need to re-arrange the source.
Context type
In

js-logger/src/logger.d.ts

Line 21 in 34f8dc5

level: ILogLevel;

it is stated that the context's log level is stored in a level property.

But in the current implementation I can find references to filterLevel (

js-logger/src/logger.js

Line 59 in 34f8dc5

this.setLevel(defaultContext.filterLevel);

) as well as level (

js-logger/src/logger.js

Line 212 in 34f8dc5

if (context.level === Logger.TIME) {

and

js-logger/src/logger.js

Line 118 in 34f8dc5

logHandler(msgArgs, merge({ level: level }, this.context));

).

Would you have any objections to just always use filterLevel (which seems to be the externally used one at the moment)?

tp · 2018-01-31T14:32:21Z

For 2) I would propose the following: Removing filterLevel and just use level in context/IContext, as that is what's currently tested:

js-logger/test-src/loggertests.js

Line 32 in 34f8dc5

assert.strictEqual(this.calls[0].context.level, logger.TRACE);

tp · 2018-01-31T15:34:21Z

Wondering what's different between my local test (just opening the test index in the browser) and Travis's. 🤔

jonnyreeves · 2018-01-31T20:27:20Z

Thank you @tp, you are an open source hero! I am on mobile, but from looking at your diff I believe you need to modify the gulpfile to run tsc so that the tests execute against the generated JavaScript.

Gulp is probably quite passé/unneeded now so feel free to migrate to npm scripts if that suits you better.

Thanks again

tp · 2018-02-01T07:29:03Z

Tests now pass on Travis 🙃

Still need to test/add the capability to run the library on Node.js, as I think that was possible before. (I have never written such an export like
https://github.com/jonnyreeves/js-logger/blob/34f8dc5e5ea2208479181b8d8871e1422c117ba1/src/logger.js#L260:L276

But I think that should be possible.

The only other remaining issue is the gulp git plugin, which crashes on require for me locally as well as on Travis. But as you suggested we can probably move away from it altogether. yarn publish does a fine job of tagging in Git.

jonnyreeves

Couple of nits, otherwise awesome work.

jonnyreeves · 2018-02-01T07:39:39Z

src/ContextualLogger.ts

+// Inner class which performs the bulk of the work; ContextualLogger instances can be configured independently
+// of each other.
+export class ContextualLogger implements ILogger {
+  public readonly context: IContext;


Any reason for making the context public?

The global logger's context is used when creating a new logger in https://github.com/tp/js-logger/blob/077cea9477c2c3a225d301dc98190c67994fe413/src/JSLoggerDefaults.ts#L46:L52

jonnyreeves · 2018-02-01T07:40:36Z

src/ContextualLogger.ts

+  // Changes the current logging level for the logging instance.
+  public setLevel(newLevel: ILogLevel) {
+    // Ensure the supplied Level object looks valid.
+    if (newLevel && "value" in newLevel) {


Not sure we need this runtime check now that we are using types.

Yep, that would be obsolete by now :) Neither null / undefined nor a non-matching interface could be passed here from strict TypeScript code.

jonnyreeves · 2018-02-01T07:46:05Z

src/logger.ts

+export const Logger: JSLoggerExportType = { ...boundGlobalLoggerFunctions, ...JSLoggerDefaults };
+
+// For those that are at home that are keeping score.
+// Logger.VERSION = "1.4.1";


We should continue to export the version here (or remove this an make a breaking change in 2.0)

Yeah, I think we can definitely keep the version. Just need to add it to the type.

jonnyreeves · 2018-02-01T07:47:00Z

src/datatypes.ts

 }

-declare var Logger: ILogger;
-export = Logger;
+export type JSLoggerExportType = IJSLoggerDefaultsType & ILogger;


Not a fan of this class name, why not just JSLogger?

👍 library name makes sense here, I agree

jonnyreeves · 2018-02-01T07:48:30Z

src/datatypes.ts

+  enabledFor(level: ILogLevel): boolean;
+}
+
+export interface IJSLoggerDefaultsType {


I think this would be a good oppertunity to make the LogLevel's a publicly exported enum, we can do this in a subsequent refactor if you want to reduce the amount of change in this PR.

Yeah, using TypeScript enums would be awesome :)

I'll see if we can utilize them in a way that would still keep them on the IJSLoggerDefaultsType, such that the tests pass without any modifications.

@jonnyreeves I think that would work. Would you be fine with a string based enum to preserve the human readability when it puts up during debugging? Put follow-up PR might be best, I hope no-one is using the internals of the log level interface.

jonnyreeves · 2018-02-01T07:50:24Z

tsconfig.json

+        "strict": true,
+        "removeComments": false,
+        "preserveConstEnums": true,
+        // "outFile": "./lib/logger.js",


remove this line.

jonnyreeves · 2018-02-01T07:53:00Z

The only other remaining issue is the gulp git plugin, which crashes on require for me locally as well as on Travis. But as you suggested we can probably move away from it altogether. yarn publish does a fine job of tagging in Git.

I've not used yarn, but I have no strong feelings on this one - happy to do whatever works :)

tp · 2018-02-02T13:52:35Z

@jonnyreeves Thanks for your input! I'll clean this up and finalize the PR.

…clusion in code

tp · 2018-02-02T15:25:10Z

@jonnyreeves I have implemented changes based on your comments and also added a new TypeScript test project to ensure that the library can actually be used in TypeScript as we intent it to :)

tp · 2018-02-02T15:29:13Z

The only remaining issue I see would be direct links to the minified library version as suggested in the README: <script src="https://cdn.rawgit.com/jonnyreeves/js-logger/master/src/logger.min.js"></script>.

Can we break these? Not sure what Github's policy is on those, but if it's generally accepted we should probably keep them for the foreseeable future.

In that case, do we keep the 1.4.1 file there for maximum compatibility? Maybe adding a note in the header, that a new version is available etc.

jonnyreeves · 2018-02-08T09:28:59Z

Can we break these?

Ehhh, I'm pretty torn on this; I'm not a fan of "breaking the web", but JS dev has come a long way since 2011 when I first authored this library and blindly acquiring JavaScript snippets in this fashion is a long, long way from "best practice" in 2018.

However, in the slim chance that someone is actually (ab)using this approach we shouldn't make their website/app stop working so let's do as you propose: publish 1.4.1 file in that location with a comment in the header explaining why it's there (and remove all mentions from the README)

…work

tp · 2018-02-09T09:59:54Z

@jonnyreeves Re-added the minified library file to the src folder.

jonnyreeves · 2018-02-10T10:16:04Z

Merged into https://github.com/jonnyreeves/js-logger/tree/2.0 - let's continue development in this branch.

tp · 2018-02-12T17:45:25Z

Hi @jonnyreeves,

great that you want to do all the clean up in the branch! Good work on #75!

Is there anything left to do that I could help you with?

https://github.com/jonnyreeves/js-logger/milestone/1 currently shows no pending work.

The only remaining thing I have thought about is running the tests on BrowserStack/some other Selenium Grid, but I don't know what the scope of that would be.

Best,
Timm

First compiling port

92c3e9f

tp added 2 commits January 31, 2018 16:12

Improve code organization

544a06e

Passing tests 🎉

4e2f3fa

tp added 3 commits January 31, 2018 16:36

Remove obsolete merge utility function

935a8c5

Remove merge usage

19ebe89

Remove merge usage

182cd2f

tp added 2 commits February 1, 2018 07:53

Attempt to fix Travis test setup

ae61237

Add linter, fix tests

077cea9

jonnyreeves reviewed Feb 1, 2018

View reviewed changes

tp added 5 commits February 2, 2018 15:53

Rename main library export type to JSLogger, fix package version in…

5131655

…clusion in code

Add TypeScript consumption test

d44973b

Remove unused line in tsconfig.json

0a90da7

Remove check which is now obsolete when using strict TypeScript

fbfba8c

Update README to point to the unpkg CDN for the simple import

b12e326

tp added 2 commits February 9, 2018 10:56

Add logger.min.js to src directory, so outside imports continue to …

8793499

…work

Add note to fallback logger.min.js file

a5a82cc

jonnyreeves closed this Feb 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port to TypeScript #71

Port to TypeScript #71

tp commented Jan 31, 2018

tp commented Jan 31, 2018

tp commented Jan 31, 2018

jonnyreeves commented Jan 31, 2018

tp commented Feb 1, 2018

jonnyreeves left a comment

jonnyreeves Feb 1, 2018

tp Feb 2, 2018

jonnyreeves Feb 1, 2018

tp Feb 2, 2018

jonnyreeves Feb 1, 2018

tp Feb 2, 2018

jonnyreeves Feb 1, 2018

tp Feb 2, 2018

jonnyreeves Feb 1, 2018

tp Feb 2, 2018

tp Feb 2, 2018

jonnyreeves Feb 1, 2018

jonnyreeves commented Feb 1, 2018

tp commented Feb 2, 2018

tp commented Feb 2, 2018

tp commented Feb 2, 2018

jonnyreeves commented Feb 8, 2018

tp commented Feb 9, 2018

jonnyreeves commented Feb 10, 2018

tp commented Feb 12, 2018

Port to TypeScript #71

Port to TypeScript #71

Conversation

tp commented Jan 31, 2018

tp commented Jan 31, 2018

tp commented Jan 31, 2018

jonnyreeves commented Jan 31, 2018

tp commented Feb 1, 2018

jonnyreeves left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonnyreeves commented Feb 1, 2018

tp commented Feb 2, 2018

tp commented Feb 2, 2018

tp commented Feb 2, 2018

jonnyreeves commented Feb 8, 2018

tp commented Feb 9, 2018

jonnyreeves commented Feb 10, 2018

tp commented Feb 12, 2018