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

Inheritance class becomes TypeError in load order. #27

Closed
jakenjarvis opened this issue Feb 13, 2019 · 13 comments
Closed

Inheritance class becomes TypeError in load order. #27

jakenjarvis opened this issue Feb 13, 2019 · 13 comments

Comments

@jakenjarvis
Copy link

Just because the inherited class has a different loading order(alphabetical), it becomes "TypeError: Cannot read property 'prototype' of undefined". Screen shot

I created a sample project to convey this.

Expected Behavior

case OK:
https://github.com/jakenjarvis/clasp-ts-sample/tree/master/IssueSample/Issue01/ProjectNormal

Actual Behavior

case NG:
https://github.com/jakenjarvis/clasp-ts-sample/tree/master/IssueSample/Issue01/ProjectAbnormal

TypeError: Cannot read property 'prototype' of undefined (line 14, file "AClass")

Steps to Reproduce the Problem

  1. Swap the class names of AClass and BClass.
  2. Oh wonder, it becomes TypeError.

Specifications

  • Windows 10 64bit
  • node v10.15.0
  • yarn 1.13.0
  • @google/clasp 1.6.3
  • @types/google-apps-script 0.0.36

I posted the same issue here(Clasp#527). I was urged to instead Issue to ts2gas.

@grant
Copy link
Owner

grant commented Mar 10, 2019

Can you please put the code that is wrong in the GitHub issue?

Please provide a simple test case.

@jakenjarvis
Copy link
Author

@grant thank you for confirmation. I'm not good at English, so it is difficult to explain. So I decided to show the source code. My explanation may not have been enough.

This sample presented by me is a simple test case. Please also check the README.

If you want to build a project from scratch, please create the following class configuration.
Be sure to check the operation on GAS. An error occurs at run time.

Create a normal project

AClass.ts

export class AClass {
  // (omission)
}

BClass.ts

import { AClass } from './AClass';
export class BClass extends AClass {
  // (omission)
}

Create a abnormal project

Swap class names(file names). Specifically, it is as follows. The difference is only to reverse the parent-child relationship.
As a result, this will reverse the load order. (It appears to be loaded in alphabetical order)

AClass.ts

import { BClass } from './BClass';
export class AClass extends BClass {
  // (omission)
}

BClass.ts

export class BClass {
  // (omission)
}

Perhaps we need a function to automatically determine load order by parsing import. I currently avoid the problem by devising the class name, but I strongly hope that loading order will be improved.

@grant
Copy link
Owner

grant commented Mar 12, 2019

I cannot debug your project's code. Please tell me the exact problem with ts2gas.
There's a config in clasp to change the load order if you need that.

@jakenjarvis
Copy link
Author

jakenjarvis commented Mar 13, 2019

There seems to be a misunderstanding, but I created a sample repository just to report you this issue.

I know that filePushOrder exists in the config. It is actually specified in this sample, but it does not seem to work effectively. Also, when the number of files increases, manual priority setting is very troublesome.(I know I don't have to specify all the files)

ProjectAbnormal/.clasp.json

It looks like it is loaded in alphabetical order at runtime on GAS, regardless of the order in which Clasp pushes. In other words, class A is always load before class B. This is the issue.
This issue is highlighted if inheriting classes and separating class definition files.

@PopGoesTheWza
Copy link
Collaborator

@jakenjarvis can you please share a link to the gas project that I can inspect?

Also I need to ask you, why do you use import and export statements?
In a gas environment, export is only useful but when creating a gas library, while import can be useful to reference a *.d.ts which expose definition of a gas library.

If the purpose is to organise your gas code in separate namespaces, I would advise on using the namespace statements and forget the import/export approach.

@jakenjarvis
Copy link
Author

@PopGoesTheWza Thank you for your reply.
This sample presented by me is a simple test case. Please also check the README.

why do you use import and export statements?

I forgot the details, but I couldn't get it to work that way. Maybe my understanding was lacking, but the namespace statements were different from those of C# I had imagined.
After all, when creating a large project, I could not determine which approach would be appropriate, and decided to use import/export statements.
So far, no issues other than this issue have occurred.

The main reason I would like to use TypeScript is to do object-oriented programming. Managing files separately for each class is a common form.
If I were presented with a sample of the recommended approach in this case before I used Clasp, I would not have got lost.

@PopGoesTheWza
Copy link
Collaborator

PopGoesTheWza commented Mar 13, 2019

@jakenjarvis my knowledge of TypeScript for Google Apps Script is still empirical. Still, from my experience, export and import should be restricted to producing and consuming gas libraries respectively.

With a "standard" gas project with multiple *.ts files resulting in as many *.gs scripts, always bear in mind that all your script get added to the same global namespace (there is not distinct namespace per .gs file). To achieve some code containment, my experience is that the namespace statement bring the best solution with the less effort.

To illustrate export use, this project define a gas library. The exported statements (classes, enums, functions, variables) will exposed in the gas library namespace:

https://github.com/PopGoesTheWza/swgoh-help-api

To illustrate namespace usage as well as import (the latest being to correctly address exposed statements from the gas library):

https://github.com/PopGoesTheWza/swgoh-tb-sheets

All the above may or may not address your original issue. I will try to give a closer look later on.

@jakenjarvis
Copy link
Author

Hi, @PopGoesTheWza

All the above may or may not address your original issue.

Using a namespace statement will probably not improve the issue.
It's not a TypeScript issue, but a GAS load order issue. A solution is needed for either Clasp or ts2gas.

export and import should be restricted to producing and consuming gas libraries respectively.

I have long been familiar with object-oriented programming in other programming languages, and most of my source code is class definitions and Interface definitions. Even when all script files are added to the global namespace, code containment has already been achieved. So I'm not very worried about it.

I know this is changing the subject, I read the source code of your project lightly. There are many things to learn and it was very helpful. Thank you.
Especially when I read this, I thought, "Oh, I see."

var client = new swgohhelpapi.module.exports.Client(settings);

I was a little surprised that I could access it this way, because I was not consciously reading at the source code after transpile. I did not think about this deeply.

var exports = exports || {};
var module = module || { exports: exports };

For me, this existed without thinking. XD
If the Clasp README explains this, it may be more helpful. (Only I could not find it?)

@jakenjarvis
Copy link
Author

Hi, @PopGoesTheWza @grant

I tried a sample that uses namespace statements, but still this issue is not solved. And I tried a couple of different ways of writing, but they all had the same issue.
Please check each of the following. Please open each GAS script and execute myFunction in code.ts.

This proves that there is no problem in the way of writing.


Issue01

This is the first sample I posted.

ProjectNormal/src/AClass.ts

export class AClass {
  // (omission)
}

ProjectNormal/src/BClass.ts

import { AClass } from './AClass';
export class BClass extends AClass {
  // (omission)
}

Issue01-2

Created a project excluding import and export.

ProjectNormal/src/AClass.ts

class AClass {
  // (omission)
}

ProjectNormal/src/BClass.ts

class BClass extends AClass {
  // (omission)
}

Issue01-3

Created a project each was used with a separate namespace statement.

ProjectNormal/src/AClass.ts

namespace nameA {
  export class AClass {
    // (omission)
  }
}

ProjectNormal/src/BClass.ts

namespace nameB {
  export class BClass extends nameA.AClass {
    // (omission)
  }
}

Issue01-4

Created a project each was used with a same namespace statement.

ProjectNormal/src/AClass.ts

namespace nameAB {
  export class AClass {
    // (omission)
  }
}

ProjectNormal/src/BClass.ts

namespace nameAB {
  export class BClass extends nameAB.AClass {
    // (omission)
  }
}

@PopGoesTheWza
Copy link
Collaborator

Hello @jakenjarvis
I had time to review your (nice) test code and here is my reasoning:

  "filePushOrder": ["src/BClass.ts", "src/AClass.ts", "src/code.ts"]
  • after running clasp push, in the script editor, script files order do not comply with the filePushOrder directive:
    image

Thus my conclusion is:

  • the transpiled code is correct
  • since ts2gas only works (at least currently) on a per file basis, there is nothing to change there
  • the filePushOrder directive in @google/clasp is either broken or misleading in its description: Specifies the files that should be pushed first, useful for scripts that rely on order of execution. All other files are pushed after this list of files.

@grant please review the above and advise. My opinion would be to open a @google/clasp issue referencing this one here (so that current work is not lost)

@jakenjarvis
Copy link
Author

@PopGoesTheWza Thank you for your code review. I felt the issue was finally reported to you.

This is called hoisting and works as designed

Yes I know it. So I pointed out that the loading order is the issue.

after running clasp push, in the script editor, script files order do not comply with the filePushOrder directive:

I did not look at it carefully.
If filePushOrder is not working properly, there was a issue of path interpretation, so it may be affecting.
The reason I keep using Clasp1.6.3 is because Clasp does not work properly between 1.7.0 and 2.0.1 on Windows. (I have not tried Clasp 2.1.0 yet)

I posted the same issue here(Clasp#527). I was urged to instead Issue to ts2gas.

Currently this issue status is closed but it should be reopened.

@PopGoesTheWza
Copy link
Collaborator

Thanks @jakenjarvis
I am not an expert on @google/clasp and merely just a contributor on ts2gas
I did my tests with clasp 2.1.0 & ts2gas 1.6.1 and experienced the same mis-ordering issue.

@jakenjarvis
Copy link
Author

Thanks @PopGoesTheWza
This issue closes and continues with issue of Clasp.

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

3 participants