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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TypeScript typings for core & webpack #247

Open
wants to merge 3 commits into
base: release-2.0
Choose a base branch
from

Conversation

zcei
Copy link
Collaborator

@zcei zcei commented Jan 7, 2018

Hej guys 馃憢

I started tackling #246, for now only @webpack-blocks/core & @webpack-blocks/webpack to have a foundation, so we can discuss naming etc.

Afterwards we just need to add the proper index.d.ts to the blocks.

If I'm not mistaken there shouldn't be other changes necessary, at least VSCode gave me type hints in the sample app already, but I'll list some here for research:

  • add typings to package.json(should be obsolete, as we comply to index.d.ts convention)
  • add index.d.ts to webpack-blocks which exports types from sub-modules (should be infered from sub-module typings)
  • find out which @types/ needs to be installed manually

@andywer
Copy link
Owner

andywer commented Jan 9, 2018

Thanks for starting work on this, @zcei! Will have a deeper look into it :)

export { createConfig, group, env, match }

export function addPlugins(plugins: Plugin[]): WebpackBlock
export function customConfig(configSnippet: Configuration): WebpackBlockUpdater
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually returns a WebpackBlock

Copy link
Owner

Choose a reason for hiding this comment

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

I think we have to agree upon some terminology. The term block, for instance, has always been a bit fuzzy. Providing explicit types is probably the ideal time to define it properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially I was thinking that a block is a self-contained, configurable portion of a Webpack Configuration - so actually entryPoint is the block, not entryPoint(), which could be a BlockInstance or similar.

Then a block is everything that has the signature (...args: any[]): BlockInstance, where BlockInstance = (current) WebpackBlock

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough. I would follow your argumentation here, @zcei. BlockInstance is not very catchy, but should do for now.

@andywer
Copy link
Owner

andywer commented Jan 30, 2018

We need to discuss terminology here, since most terms haven't ever been clearly defined.
Current state of affairs is:

Initially I was thinking that a block is a self-contained, configurable portion of a Webpack Configuration - so actually entryPoint is the block, not entryPoint(), which could be a BlockInstance or similar.

Then a block is everything that has the signature (...args: any[]): BlockInstance, where BlockInstance = (current) WebpackBlock

If someone has a better naming suggestion for BlockInstance, feel free to share it here 馃槈

@andywer
Copy link
Owner

andywer commented Jan 30, 2018

Another term that might be discussable is WebpackBlockUpdater.

Maybe it's rather a WebpackConfigUpdater: It's actually so generic that it's not webpack-blocks-specific. And this name might transport more information. Objections?

@andywer
Copy link
Owner

andywer commented Jan 30, 2018

Update: Let's better discuss this in the issue (#246) than in the PR.

@andywer andywer mentioned this pull request Jan 30, 2018
@vlad-zhukov vlad-zhukov mentioned this pull request Apr 21, 2018
17 tasks
@vlad-zhukov vlad-zhukov added this to the 2.0.0 milestone Apr 21, 2018
@vlad-zhukov vlad-zhukov changed the base branch from master to release-2.0 April 21, 2018 13:32
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.

None yet

3 participants