Skip to content
This repository has been archived by the owner on Jun 28, 2021. It is now read-only.

[Fixes #12] Add Browserify to build #18

Merged
merged 2 commits into from
Jan 2, 2017
Merged

Conversation

bluepichu
Copy link
Collaborator

@bluepichu bluepichu commented Jan 2, 2017

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:

let textSample = new MultiStyleText.MultiStyleText("It's sad, but required.");

@bluepichu bluepichu requested a review from tleunen January 2, 2017 03:17
Copy link
Owner

@tleunen tleunen left a 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 {
Copy link
Owner

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?

Copy link
Collaborator Author

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",
Copy link
Owner

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

@tleunen tleunen merged commit cf527eb into tleunen:master Jan 2, 2017
@bluepichu bluepichu deleted the iss12-fix branch January 16, 2017 05:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants