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

Added jointjs typings #301

Merged
merged 2 commits into from
Apr 10, 2016
Merged

Added jointjs typings #301

merged 2 commits into from
Apr 10, 2016

Conversation

CaselIT
Copy link
Contributor

@CaselIT CaselIT commented Apr 8, 2016

Filling in these fields will make it easier for us to review:

Typings URL: [E.g. https://github.com/CaselIT/typings-jointjs]
Source URL: [E.g. https://github.com/clientIO/joint]

Thanks for contributing!

@unional
Copy link
Member

unional commented Apr 8, 2016

Correction:
https://github.com/CaselIT/typings-jointjs/blob/master/index.d.ts
Does it only exposing g and v?

@CaselIT
Copy link
Contributor Author

CaselIT commented Apr 8, 2016

No it exposes also the objects dia ui shapes util and layout

@unional
Copy link
Member

unional commented Apr 8, 2016

Oh, interesting. Never know it works that way. Thanks.

@unional
Copy link
Member

unional commented Apr 8, 2016

It's ES6 -> CommonJS interop

@CaselIT
Copy link
Contributor Author

CaselIT commented Apr 8, 2016

I'm not sure I follow. Do you mean the missing exports in the namespace declaration?

@unional
Copy link
Member

unional commented Apr 8, 2016

Nope. I tested it and it is working.
jointjs is using export = syntax at index.js, and what you have is in ES6 syntax. So it is a ES6 -> CommonJS interop.

@unional
Copy link
Member

unional commented Apr 8, 2016

Not sure if that is a good idea to do so, as it might backfire as the CommonJS -> ES6 interop issue.
I think there are some ES6->CommonJS interop trick babel is doing.
But I guess this would be fine.

@blakeembrey , what do you think?

@unional
Copy link
Member

unional commented Apr 9, 2016

@CaselIT you probably aware of it, but for reference, what I'm talking about is related to:
microsoft/TypeScript#7398

@CaselIT
Copy link
Contributor Author

CaselIT commented Apr 9, 2016

I was not aware of it, thanks for pointing it out. I think I did experience something similar while writing the type definition for this library. If load this library with system.js as global the external module as it is written is incorrect, while it works when loading it as commonjs.
Still I think most people will use the ambient definitions and load the library with a script tag.
Do you have any advice to avoid the issue?

@unional
Copy link
Member

unional commented Apr 9, 2016

Still I think most people will use the ambient definitions and load the library with a script tag

Loading from script tag just means the typings needs to be ambient/global/script. So the /global/* should take care of it.

Do you have any advice to avoid the issue?

Currently no, unfortunately. That why I think the issue is massive. The best we can do is to stay as close to the shape of the source as possible.

i.e. in this case keep it in the export = syntax.

CaselIT added a commit to CaselIT/typings-jointjs that referenced this pull request Apr 10, 2016
@CaselIT
Copy link
Contributor Author

CaselIT commented Apr 10, 2016

I tried to change the type definition, did you mean like they are now? They effectively are as before.

https://github.com/CaselIT/typings-jointjs/blob/test-export/index.d.ts

If this was what you meant, wait before merging so I can update the pull request with the new commit.

@unional
Copy link
Member

unional commented Apr 10, 2016

yup. 👍

@CaselIT
Copy link
Contributor Author

CaselIT commented Apr 10, 2016

I've updated the pull request.

@unional
Copy link
Member

unional commented Apr 10, 2016

FYI Having some difficulty to merge

@CaselIT
Copy link
Contributor Author

CaselIT commented Apr 10, 2016

Can I help?

@unional
Copy link
Member

unional commented Apr 10, 2016

It's related to the repo setting I think. So need @blakeembrey's help on it.

@CaselIT
Copy link
Contributor Author

CaselIT commented Apr 10, 2016

I can re-fork the repo and risubmit the pull request if you think that it would help.

@unional
Copy link
Member

unional commented Apr 10, 2016

Thanks. But it's not just your PR. Same for all other PRs. 😏

@blakeembrey blakeembrey merged commit 0d5dc93 into typings:master Apr 10, 2016
@blakeembrey
Copy link
Member

Sorry, it appeared to be an odd restriction that comes with the enablement of the squash option. It should now be fixed for collaborators.

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