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

Multiple TS Changes and Improvements #290

Merged
merged 3 commits into from
Jun 29, 2019
Merged

Multiple TS Changes and Improvements #290

merged 3 commits into from
Jun 29, 2019

Conversation

YourWishes
Copy link
Contributor

Refer to #289
Also see #288

Also, is there a preferred prettier config? I went with --single-quote --trailing-comma es5, although it's done some aesthetic damage to my WebhookMap which could be a tad confusing to glance at, thoughts?

Pull Request Fixes

Fixed commit title to omit trailing period
Ran through prettier to correct semicolon and comma inconsistencies.

Definition Changes

Added missing property id to ICarrierService
Added Interface for ICart, ICartLineItem and IDeletedItem
Added WebhookType map, e.g. WebhookType<'products/create'> will yeild IProduct
Added various types for shipping carrier callbacks.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a348722 on YourWishes:master into ee36abe on MONEI:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a348722 on YourWishes:master into ee36abe on MONEI:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a348722 on YourWishes:master into ee36abe on MONEI:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a348722 on YourWishes:master into ee36abe on MONEI:master.

@lpinca
Copy link
Collaborator

lpinca commented Jun 27, 2019

No trailing commas please. Single quote is ok and consistent with current style.

@YourWishes
Copy link
Contributor Author

No trailing commas please. Single quote is ok and consistent with current style.

alright thanks for clearing up, I ran the defintions through prettier with the updated config, no changes to commit though.

@lpinca
Copy link
Collaborator

lpinca commented Jun 27, 2019

Rubber stamp LGTM. Are there breaking changes? I mean, do these types work with TypeScript < 3? Is there a chance that this will break people builds? (I don't use TypeScript)

@YourWishes
Copy link
Contributor Author

YourWishes commented Jun 28, 2019

The map for the webhooks uses conditional types, according to the docs these were introduced in TS 2.8.x.

Reverting back to version 2.0.10 shows that it will still compile without errors when not being used, the only time an error will be thrown is when attempting to use the new maps in a format that is unexpected.

TS 2.0.10

type T = Shopify.WebhookType<'products/create'>;
let x = request.get() as T;//T is type of 'products/create' not the expected IProduct
x.id;//Will result in an error since TS thinks x is a string

Without use of Webhook Types Map (How it's currently being handled)

let x = request.get() as IProduct;//Implicit cast of any to IProduct
x.id;//No error

TS 2.8.x+

type T = Shopify.WebhookType<'products/create'>;
let x = request.get() as T;//T is type of IProduct
x.id;//Works without error.

Benefits;

const addWebhook = <T extends WebhookTopic>(hook:T, express:Express, cb:(o:WebhookType<T>) => void) => {
  express.post(`/webhook/${hook}`, (req,res) => {
    cb(req.get() as WebhookType<T>);
  });
}

addWebhook('products/create', app, product => {
  product;//Typeof IProduct
  product.id;//No errors, returns a number.
});

addWebhook('checkouts/create', app, checkout => {
  checkout;//Typeof ICheckout
});

@lpinca lpinca merged commit 1cf1817 into MONEI:master Jun 29, 2019
@lpinca
Copy link
Collaborator

lpinca commented Jun 29, 2019

Thank you.

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.

3 participants