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

fix(queryTypes) use enums instead of object for ambient context #141

Merged
merged 4 commits into from
Nov 2, 2017

Conversation

modulitos
Copy link
Contributor

Fixes #138

DESCRIBE,
RAW,
FOREIGNKEYS,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wrong. Those are not numeric enums, they are strings. A string enum would work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixfbecker Thanks for the correction. I'm pushing up a fix now to make this a string enum.

I appreciate the feedback 👍

@@ -1,16 +1,17 @@
declare enum QueryTypes {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be exported, not declared as a global

Copy link
Contributor Author

@modulitos modulitos Oct 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that by using export default enum QueryTypes { .... but the compiler complains with the message Expression expected. [1109].

It doesn't seem like I can use const enum QueryTypes {.. followed by export default QueryTypes either, because of the error: A 'declare' modifier is required for a top level declaration in a .d.ts file. [1046].

Any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I think I can use export enum QueryTypes {... here. I'll push up a fix....

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to mirror the export style in the Sequelize library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you referring to me using export default ... instead of export ...? If so, that makes sense and I'll push up a fix soon.

If you are referring to something else in the library, then let me know if I'm missing anything.

Copy link
Collaborator

@felixfbecker felixfbecker Oct 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixfbecker I'm not sure that I can export my enum using the export = assignment without declaring my enum globally. When I try the following:

enum QueryTypes {
  SELECT = "SELECT",
  INSERT = "INSERT",
  UPDATE = "UPDATE",
  BULKUPDATE = "BULKUPDATE",
  BULKDELETE = "BULKDELETE",
  DELETE = "DELETE",
  UPSERT = "UPSERT",
  VERSION = "VERSION",
  SHOWTABLES = "SHOWTABLES",
  SHOWINDEXES = "SHOWINDEXES",
  DESCRIBE = "DESCRIBE",
  RAW = "RAW",
  FOREIGNKEYS = "FOREIGNKEYS",
}

module.exports = QueryTypes

I get the following error: A 'declare' modifier is required for a top level declaration in a .d.ts file. [1046].

And when I try:

module.exports = enum {
  SELECT = "SELECT",
  INSERT = "INSERT",
  UPDATE = "UPDATE",
  BULKUPDATE = "BULKUPDATE",
  BULKDELETE = "BULKDELETE",
  DELETE = "DELETE",
  UPSERT = "UPSERT",
  VERSION = "VERSION",
  SHOWTABLES = "SHOWTABLES",
  SHOWINDEXES = "SHOWINDEXES",
  DESCRIBE = "DESCRIBE",
  RAW = "RAW",
  FOREIGNKEYS = "FOREIGNKEYS",
}

I get this error: Expression expected. [1109].

Since enums are types and cannot be assigned within an expression, this seems to be the same reason why we are using export interface ... instead of exports = interface ....

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try

enum QueryTypes { ... }
export = QueryTypes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and I get the same A 'declare' modifier is required for a top level declaration in a .d.ts file. [1046] error

RAW: 'RAW',
FOREIGNKEYS: 'FOREIGNKEYS'
};
export enum QueryTypes {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not how it is in the source file.
Please add back export = QueryTypes at the bottom.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described in my comment here: #141 (comment) using export = QueryTypes causes a compiler error.

It seems like the only other option is to declare QueryTypes globally using declare enum QueryTypes as done in this commit.

I am fairly new to Typescript, so perhaps I'm missing something. Why are we trying to mirror the export style in the Sequelize library?

@felixfbecker felixfbecker merged commit 8bed52a into types:master Nov 2, 2017
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

Successfully merging this pull request may close these issues.

None yet

2 participants