Drop `index.js` in every component folder

aeternity / aepp-components

deprecated: aepp-components to be used in all aepps.

ISC License

41 stars 14 forks source link

Drop `index.js` in every component folder #168

Closed davidyuk closed 5 years ago

davidyuk commented 5 years ago

Based on #124

sadiqevani commented 5 years ago

Hey @davidyuk

Why have you removed the index.js I believed we agreed not to make major changes on the components.
Naming new components with [component name]2 is not a good practice, I would prefer to have the new component as the [component name] and the old component [component name]Old or something similar that makes sense, I leave that up to you
for now lets not refactor things! I think the state of the project here is okay. What is needed mostly now are adding components rather than changing the structure.

davidyuk commented 5 years ago

Why have you removed the index.js

You mean in every component folder? They are not necessary, nothing is broken without them.

Naming new components with [component name]2 is not a good practice

I agree that changes were too hasty, let's keep only yours components exported by default, but in the future, we need to merge the functionality of components with the same name.

for now lets not refactor things! I think the state of the project here is okay.

This code is already implemented, it removes 450loc of useless code and prevents from a writing of useless code in future, you can't reject this PR just because it is refactoring.

sadiqevani commented 5 years ago

Hey @davidyuk

I'm not saying i'm rejecting it, I just don't agree with some of the changes.

You mean in every component folder? They are not necessary, nothing is broken without them.

Nothing is broken, I know, but also functionality that I want to have in the component repository is being removed. The per-component export that I think solves some specific use-cases where developers can include only a single component are not being implemented.

Not everyone has implemented tree-shaking, and this is an open source project, so we need to support a wide use-case.

So the index.js acts as a middle man for different levels of export.

I would like to postpone the merging of this pull-request for later, since I don't see any immediate value for now.

davidyuk commented 5 years ago

Nothing is broken

So, actually, I have broke per-component build, I forgot to check it. It is fixed in the latest commit.

and this is an open source project, so we need to support a wide use-case

Most of the component packages are open source, have you seen/used a similar per-component build process anywhere else? Vue CLI is intentioned to be used for the building of components libraries, they propose to build Vue components into async web components, I think it suits for the wider set of use-cases than your approach, and it can be configured much simpler, and it will be maintained by vue-cli team.

I don't see any immediate value for now

Future PRs (for example #134) can be simpler because won't be necessary to write index.js file for every component. Postponing of this PR will demand resources to keep it up to date, to be able to merge it in future.

sadiqevani commented 5 years ago

@davidyuk Lets merge this pull-request as well! I like the solution for the index.js per-component output.

Can you fix the conflicts?

davidyuk commented 5 years ago

Sure! The branch is rebased on the top of develop.