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

feat: (WIP) started conversion of tapable to TypeScript #13

Closed

Conversation

TheLarkInn
Copy link
Member

Still very work in progress. One thing to note, is that TS doesn't support ..args that are not the final parameter. There is an ugly workaround that would have to be considered. microsoft/TypeScript#7347

@jayphelps
Copy link

jayphelps commented Sep 21, 2016

not ideal, but the API could change to be Promise-based instead of callback based.

applyPluginsAsyncSeries(name: string, ...args: Array<any>): Promise

That's probably an entire bikeshedding session on its own though...

/ducks out before someone throws a shoe

@TheLarkInn
Copy link
Member Author

I mean promises would be really cool is just that it would be a ginourmous breaking change.

return current;
}

protected applyPluginsWaterfall0(name: string, init: any) {
Copy link

Choose a reason for hiding this comment

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

the old style that naming function with number is very confusing. better to be self-explained.

Copy link
Member Author

Choose a reason for hiding this comment

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

So changing the actual name of the function would require a huge changes to webpack/core, but once I've gotten this fully migrated, I can add a comment to explain how some are ussing call and some use apply thus the strange name.

@e-cloud
Copy link

e-cloud commented Sep 22, 2016

promise is good. Breaking change is inevitable when introducing awesome feature in most of time.

As webpack 2 will be incompatible with webpack 1. Introducing breaking change would affect webpack 2 basically

@e-cloud
Copy link

e-cloud commented Sep 22, 2016

One thing worth mentioned is documentation. Especially, the apply method overlay the built-in one worth documenting why doing this, what this do, how to use it.

As webpack grows so popular, the core concept is one of the most important thing to be clarified, for further improvement.

@TheLarkInn
Copy link
Member Author

I agree with you. Most users don't even understand Tapable or the entire webpack internals. It certainly is an opprotunity to fix that through code quality.

@TheLarkInn
Copy link
Member Author

Types are on hold at the moment. Would require potentially a design change especially with delegation.

@TheLarkInn TheLarkInn closed this Jan 28, 2017
@TheLarkInn TheLarkInn deleted the feature/tapable_typescript branch January 28, 2017 04:52
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