Issue With Namespaces?

[dsherret]

Issue With Namespaces?

[dsherret]

@sfrooster since it's a tree, you will have to get the namespace first then get the class within the namespace. So something like this:

sourceFile.getNamespace("NamespaceName")!.getClasses(); // or whatever way to get the namespace

Perhaps an easy way could be added for getting all classes including ones within namespaces and functions too.

Also, maybe .getClasses() is not appearing on NamespaceDeclaration because of that auto-complete issue.

[sfrooster]

Yep, it was the same as the previous issue. getClasses didn't appear at first but eventually, when I went back, it did appear. I'm going to close the previous issue - it's probably a vscode issue.

I'm getting "Error: A child syntax list was expected." when looking for classes on that namespace. That namespace doesn't have any classes but does have another module. Anyway, I'll play with it a bit more and probably end-up closing this one too.

FYI - your lib is WAY easier to use than so far than the straight compiler api!

[dsherret]

Do you have some code that reproduces that error? That definitely shouldn't happen. It might be something I didn't take into account.

Thanks! I know... the compiler api is very difficult to use. Sometimes a simple function in this library will be a ton of code using the compiler api.

[sfrooster]

I'm just starting to play with you lib so I can figure out how best to use it so the code I'm currently running looks like:

import Ast from "ts-simple-ast";
import * as ts from "typescript";

const ast = new Ast({
    tsConfigFilePath: "tsconfig.json"
});

ast.addSourceFiles("msgs/**/*{.d.ts,.ts}");

ast.getSourceFiles().forEach(sf => {
    console.log(sf.getFilePath());

    let nscc: number[] = [];
    let ns = sf.getNamespaces();
    ns.forEach(n => {
        nscc.push(n.getClasses().length);
    });
    let classes = sf.getClasses();
    let ifaces = sf.getInterfaces();

    console.log(`c: ${classes.length}, i: ${ifaces.length}, n: ${nscc}`);
    console.log("=================");
});

An example of a source file it's having an issue with is:

export namespace Demo.UserProfiles.Commands {
    export class CreateUserProfile extends Message {
        constructor();
        constructor(Email: string, FirstName: string, LastName: string, SecurityToken: string);
        constructor(public Email: string = "", public FirstName?: string, public LastName?: string, public SecurityToken?: string) {
            super();
        }
    }
}

Specifically this line "nscc.push(n.getClasses().length);" is choking on the UserProfiles piece of "export namespace Demo.UserProfiles.Commands".

I hope that helps!

[dsherret]

Yes, it does! Thank you very much!

So I never thought about namespaces that have dots in the name. What the AST does is this:

ModuleDeclaration - Demo
  - ModuleDeclaration - UserProfiles
    - ModuleDeclaration - Commands
      - ModuleBlock
        -SyntaxList  
          - ClassDeclaration

If you do .getName() on the namespace you'll see it just return Demo. An ugly work around is to do:

const innerNamespace = ns.getFirstChildByKindOrThrow(ts.SyntaxKind.ModuleDeclaration)
    .getFirstChildByKindOrThrow(ts.SyntaxKind.ModuleDeclaration) as NamespaceDeclaration;
nscc.push(innerNamespace.getClasses().length);

I'm working on a fix right now though, but I'll also need to fix what .getName() returns.

[sfrooster]

I think I can work with that until you get a fix in. I'll need to tweak it to handle variable length dot-separated namespaces, but I'm sure that's doable.

In reality, I think your code is mostly right. When tsc compiles that it treats it as three nested modules or whatever:

var Demo;
(function (Demo) {
    var UserProfiles;
    (function (UserProfiles) {
        var Commands;
        (function (Commands) {
            var CreateUserProfile = (function (_super) {

So the name of the first Node is actually "Demo". getClasses in that case should just return an empty list but then getModules / getNamespaces (I'm not sure which is appropriate) should return "UserProfiles" - don't you think?

I'll defer to you but that seems correct to me.

[dsherret]

Fixed in 0.28.2.

In this case, I think it makes sense to return the classes/interfaces/functions/namespaces/etc within the ModuleBlock for every ModuleDeclaration in the tree. I think it's very unlikely that someone would want to do ns.getNamespaces()[0].getNamespaces()[0].getClasses() just in order to get the classes within the block and that would be annoying. If someone really wants to traverse down the tree on their own, they can always do what I mentioned in my last post, by doing .getFirstChildByKind(ts.SyntaxKind.ModuleDeclaration).

I'm going to think about what to do with .getName() tomorrow. I think you're right that it should return Demo because that's what's on the .name property, but I may want to just make the common case the default (fully qualified name?) and then allow people to specify what kind of name they want if not.

[sfrooster]

I have switched over to java land briefly but will be back in TS world a little later and I'll give it a try.

[sfrooster]

I'm not sure exactly which direction you're headed in but I did play around with this some more this evening. I was ultimately able to get the correct result, but it was mixed in with misleading results. This probably gets to your getName question because getClasses was returning a class buried three levels deep when I called it at each level. Unless getClasses called at a top level can indicate the correct level for each returned class, I'd think it would be better if it was only returned at the correct level. As it is, I'm not able to correctly determine the namespace for the class.

[dsherret]

Given the following code:

export namespace Demo.UserProfiles.Commands {
    export class Class1 {
    }

    export namespace InnerNamespace {
        export class Class2 {
        }
    }
}

export namespace Demo.UserProfiles {
    export class Class3 {
    }
}

Result of getClasses(), getNamespaces(), etc...

I'm sure most people would expect it to behave the following way (as you did when you initially tried it out and I would have thought would work myself):

const namespaces = sourceFile.getNamespaces();
namespaces.length; // 1

// first namespace
const ns1 = namespaces[0];
ns1.getClasses();      // 1, Class1
const ns1Inner = ns1.getNamespaces()[0];
ns1Inner.getClasses(); // 1, Class2

// second namespace
const ns2 = namespaces[1];
ns2.getClasses();      // 1, Class3

I believe that's now the current behaviour. I usually try to mirror the AST exactly in most cases, but I view these .getClasses() as helper methods that help get things more easily. In this case, .getClasses() can be viewed as getting all the classes within the braces.

Dot Notation Names

I've been thinking about how the name should work on namespaces. I'm thinking of replacing the current name methods:

// current
getNameNode(): Identifier;
getName(): string;
rename(newName: string): this;

// final - renamed to somewhat reflect that these are different nodes
getNameNodes(): Identifier[];
getFullName(): string;
setFullName(newFullName: string): this;

So in the example above the following code would happen:

ns1.getFullName(); // "Demo.UserProfiles.Commands"
ns1.getNameNodes()[0].getText(); // "Demo"
ns1.getNameNodes()[0].rename("NewDemo"); // renames usages of "Demo" to "NewDemo"
ns1.setFullName("Demo.UserProfiles.New.Commands"); // inserts "New"

Overall, with a statement like "namespace Demo.UserProfiles.Commands {}", I would rather have to explain to a couple of people why it's the way I outlined above than to explain to everyone about the underlying AST structure. Maybe I'm completely wrong about this though... I'm still thinking about it.

What are your thoughts?

[sfrooster]

I've updated to 0.30.0 and been playing around with it. What it really comes down to is being able to properly work with namespaces/modules. As far as I can tell getClasses does return all the classes encapsulated by the given namespace / module.

It also seems like you can find a class's proper namespace if you start with the class and work your way up through getParent - I've been able to do that manually but it remains to be seen how easily I can automate that (probably not too bad).

The difficult part is trying to take a top-down namespace approach and be accurate in identifying where you are in the namespace.

Given code like:

export namespace Demo.UserProfiles.Commands {
    export class CreateUserProfile extends Message {
        public static readonly Version: string = "1.0";   // version to pass to EDAProperties
        public static readonly TypeID = "OT.Messages.Demo.UserProfiles.Commands.CreateUserProfile";

        public get Key(): number[] {
            return stringToByteArray(this.Email!);
        }

        constructor();
        constructor(Email: string, FirstName: string, LastName: string, SecurityToken: string);
        constructor(public Email: string = "", public FirstName?: string, public LastName?: string, public SecurityToken?: string) {
            super();
        }
    }

    export namespace Nested {
        export class CreateUserProfile2 {
            public x: number = 0;
        }
    }
}

You need the two of these

let nsc = ns.getNamespaces();
let cok = ns.getChildrenOfKind<NamespaceDeclaration>(ts.SyntaxKind.ModuleDeclaration);

to scan through the entire "namespace" and if you process the entire tree recursively, what you'll find is that "Nested" is identified as a sub namespace/module of each part of the original namespace so "Demo", "UserProfiles", and "Commands" when in reality it should only be a sub namespace/module of "Commands".

Here's my messed-up code:

import Ast from "ts-simple-ast";
import { ClassDeclaration, NamespaceDeclaration, Node } from "ts-simple-ast";
import * as ts from "typescript";
import * as _ from "lodash";
import * as util from "util";

const ast = new Ast({
    tsConfigFilePath: "tsconfig.json"
});

ast.addSourceFiles("msgs/**/*{.d.ts,.ts}");

let classNames: string[] = [];

ast.getSourceFiles().forEach((sf, idx) => {
    if (idx === 0) {
        console.log(sf.getFilePath());
        processNS2(sf.getNamespaces());
    }
});

function processNS2(nss: NamespaceDeclaration[], level: number = 0) {
    let prefix = _.padStart("", level * 4, " ");

    nss.forEach(ns => {
        classNames.push(...ns.getClasses().map(c => c.getName()));
        classNames = _.uniq(classNames).sort();
        console.log(prefix, `CLASSES: ${classNames.join(", ")}`);

        console.log(prefix, `NS: ${ns.getName()}`);
        ns.getClasses().forEach(c => {
            console.log(prefix, `C: ${c.getName()}`);
        });

        let nsc = ns.getNamespaces();
        let cok = ns.getChildrenOfKind<NamespaceDeclaration>(ts.SyntaxKind.ModuleDeclaration);

        console.log(prefix, `NSC: ${nsc.map(nsd => nsd.getName()).join(", ")}`);
        processNS2(nsc, level + 1);
        console.log(prefix, `COK: ${cok.map(nsd => nsd.getName()).join(", ")}`);
        processNS2(cok, level + 1);

        console.log(prefix, "===============");
    });
}

Which produces output like (disregard the CLASSES lines - I don't think they're useful):

Debugging with legacy protocol because Node.js v6.10.3 was detected.
node --debug-brk=16262 --nolazy lib/app2.js 
Debugger listening on [::]:16262
/Users/mhaan/AppDev/tscompile/msgs/OT/Messages/Demo/demo-messages.ts
 CLASSES: CreateUserProfile
 NS: Demo
 C: CreateUserProfile
 NSC: Nested
     CLASSES: CreateUserProfile, CreateUserProfile2
     NS: Nested
     C: CreateUserProfile2
     NSC: 
     COK: 
     ===============
 COK: UserProfiles
     CLASSES: CreateUserProfile, CreateUserProfile2
     NS: UserProfiles
     C: CreateUserProfile
     NSC: Nested
         CLASSES: CreateUserProfile, CreateUserProfile2
         NS: Nested
         C: CreateUserProfile2
         NSC: 
         COK: 
         ===============
     COK: Commands
         CLASSES: CreateUserProfile, CreateUserProfile2
         NS: Commands
         C: CreateUserProfile
         NSC: Nested
             CLASSES: CreateUserProfile, CreateUserProfile2
             NS: Nested
             C: CreateUserProfile2
             NSC: 
             COK: 
             ===============
         COK: 
         ===============
     ===============
 ===============
 CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected
 NS: Demo
 C: UserProfileCreated
 C: UserProfileRejected
 NSC: 
 COK: UserProfiles
     CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected
     NS: UserProfiles
     C: UserProfileCreated
     C: UserProfileRejected
     NSC: 
     COK: Events
         CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected
         NS: Events
         C: UserProfileCreated
         C: UserProfileRejected
         NSC: 
         COK: 
         ===============
     ===============
 ===============
 CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, ValidateUserProfileRequest
 NS: Demo
 C: ValidateUserProfileRequest
 NSC: 
 COK: UserProfiles
     CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, ValidateUserProfileRequest
     NS: UserProfiles
     C: ValidateUserProfileRequest
     NSC: 
     COK: Rpcs
         CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, ValidateUserProfileRequest
         NS: Rpcs
         C: ValidateUserProfileRequest
         NSC: 
         COK: 
         ===============
     ===============
 ===============
 CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, ValidateUserProfileRequest, ValidateUserProfileResponse
 NS: Demo
 C: ValidateUserProfileResponse
 NSC: 
 COK: UserProfiles
     CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, ValidateUserProfileRequest, ValidateUserProfileResponse
     NS: UserProfiles
     C: ValidateUserProfileResponse
     NSC: 
     COK: Rpcs
         CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, ValidateUserProfileRequest, ValidateUserProfileResponse
         NS: Rpcs
         C: ValidateUserProfileResponse
         NSC: 
         COK: 
         ===============
     ===============
 ===============
 CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, UserSignupSubmitted, ValidateUserProfileRequest, ValidateUserProfileResponse
 NS: Demo
 C: UserSignupSubmitted
 NSC: 
 COK: Web
     CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, UserSignupSubmitted, ValidateUserProfileRequest, ValidateUserProfileResponse
     NS: Web
     C: UserSignupSubmitted
     NSC: 
     COK: Events
         CLASSES: CreateUserProfile, CreateUserProfile2, UserProfileCreated, UserProfileRejected, UserSignupSubmitted, ValidateUserProfileRequest, ValidateUserProfileResponse
         NS: Events
         C: UserSignupSubmitted
         NSC: 
         COK: 
         ===============
     ===============
 ===============

It finds "Nested" under both "Demo" and "UserProfiles", for example...

[dsherret]

I've changed the behaviour a little here in 0.35.0. Now the following will happen:

export namespace Demo.UserProfiles.Commands {
    export class Class1 {
    }
}

const namespaceDec = sourceFile.getNamespaces()[0];
namespaceDec.getName(); // returns: Demo.UserProfiles.Commands
namespaceDec.setName("Demo.User.Other"); // would set the name to that... not implemented yet
namespaceDec.getNameIdentifiers(); // returns the identifiers for Demo, UserProfiles, and Commands
cons classDec = namespaceDec.getClasses()[0]; // returns: Class1's ClassDeclaration

classDec.getParentNamespace() === namespaceDec;

I believe in this case it is fine to hide some complexity here to support the simplest and what I believe to be the expected behaviour to someone who has never navigated the AST. For a user who really wants to go down through the AST here they could do that if they wanted, but I don't see the point.

[dsherret]

I'm going to close this for now. The only part that's not implemented is calling setName when dot notation is used currently or setting it to use dot notation. Let me know if there's anything I missed or if this doesn't address the issue.