-
Notifications
You must be signed in to change notification settings - Fork 39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. Why would this be wrapped into another MultiStyleText module?
Can't we just export the current MultiStyleText
class?
valign?: "top" | "middle" | "bottom"; | ||
} | ||
|
||
interface TextStyleSet { | ||
export interface TextStyleSet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary/useful to export the interfaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's required - you can't export the MultiStyleText
class if it has properties or methods that use types that wouldn't be accessible externally.
"@types/pixi.js": "^4.3.1", | ||
"typescript": "^2.1.4" | ||
}, | ||
"scripts": { | ||
"demo": "npm run build && open demo/index.html", | ||
"build": "tsc", | ||
"build": "tsc; browserify pixi-multistyle-text.ts -p tsify -s MultiStyleText -o dist/pixi-multistyle-text.js -d", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought tsc
was already compiling down to basic JS. Could you explain why browserify is required here?
Also, I'd like to find a way to remove the new wrapped module to remove the duplicationMultiStyleText.MultiStyleText
when the file is injected in a script tag instead of using a commonjs require/import for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of Browserify here is to export the module for CommonJS and AMD if either of those are being used, and otherwise defaulting to a global export. This is similar to how Pixi itself functions (and they also use Browserify to build). I was originally hoping we could get away with using TS's umd
module mode, which handles both CommonJS and AMD properly, but it doesn't default to exposing the module globally if neither is found.
I'd also like to remove the "double wrapping" problem if possible, but I couldn't find a way to do so. (In particular, I would've expected that export default class MultiStyleText
would do the trick, but that instead exports the class to MultiStyleText.default
.)
I'll keep looking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a solution. I don't love it (it's kind of a hack), but it works. See TypeStrong/tsify#105.
Turns out the fix I had before was a full fix: the
.d.ts
issues I was having were due to setup errors on my end.EDIT: No longer has any breaking changes.
The following no longer applies. I'm keeping it here so that the discussion below makes sense.
Breaking change:
MultiStyleText
is now wrapped in a module, which means that even when including this package via a<script>
tag, you need to "unwrap" it to use it: