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

Rework the project structure #501

Closed
hsablonniere opened this issue Jul 13, 2022 · 14 comments · Fixed by #512
Closed

Rework the project structure #501

hsablonniere opened this issue Jul 13, 2022 · 14 comments · Fixed by #512
Assignees
Labels
breaking-change Something that will required a major semver release maintenance Code refactoring, project structure, dev tooling (storybook, dev server, npm tasks...)

Comments

@hsablonniere
Copy link
Member

Context

All components are organized in folders that have some kind of meaning.
It can be semantic: atoms or related to the domain: env-vars.

Problems

  • Finding the right name for the folder is often cumbersome
  • This folder must be manually listed in the inputs config for rollup
  • The components are published in these folders on npm
    • Which means users need to know where to find them
    • It also means if we want to reorganize and move a component to another folder, it's a breaking change

NOTE: The folder is not used by Storybook for the left menu/tree (even if it's the same). This is done through CSF config.

Proposition

1) Component files in src/components

Example: we move src/atoms/cc-input-text.js to src/components/cc-input-text.js.

  • Where do we put the types.d.ts?
    • I guess this means every component refers to one giant common types.d.ts file.
  • I guess this means the smart definition would be mixed up with all these files

2) Component files in src/components/cc-my-component

Example: we move src/atoms/cc-input-text.js to src/components/cc-input-text/cc-input-text.js.

  • Where do we put the types.d.ts?
    • We could put all types only used by the component in a types.d.ts in the same folder as the component
    • And keep using the common types.d.ts for types used by multiple components
  • This would allow us to colocate a smart definition with a component file
    • Example: src/homepage/cc-article-list.smart.js to src/components/cc-article-list/cc-article-list.smart.js.
    • Do we have smart definition that target multiple components?
      • I don't think so
  • Do we want to colocate the story file in the component folder?
    • Example: stories/atoms/cc-input-text.stories.js to src/components/cc-input-text/cc-input-text.stories.js.
  • Do we want to colocate the markdown file used for smart definitions in the component folder?
    • Example: stories/homepage/cc-article-list.smart.md to src/components/cc-article-list/cc-article-list.smart.md.
  • Right now, we don't have automated tests for components but we may colocate the test file in this folder too

Impacts

In both cases, I propose that we publish to npm with all components directly available in the root folder.

import `@clevercloud/components/dist/cc-input-text.js`;

⚠️ This is a breaking change.

@hsablonniere hsablonniere added maintenance Code refactoring, project structure, dev tooling (storybook, dev server, npm tasks...) breaking-change Something that will required a major semver release labels Jul 13, 2022
@hsablonniere hsablonniere added this to Discuss in Project board - Clever Components via automation Jul 18, 2022
@roberttran-cc
Copy link
Member

Overall, I would go with option 2: it looks better for maintainability and understanding, especially in the long term.

And I would put everything in the same folder as the component: I could imagine having stories and/or tests in a distinct folder, as they are part of a "different" stack but I've got no true reason to do this (not convinced).

And I agree with the root folder availability.

About this, I know it is possible to explicitly specify the package's entry points but I had trouble using it in a previous project: https://nodejs.org/api/packages.html#package-entry-points

@pdesoyres-cc
Copy link
Contributor

I would choose the second option where we, as much as possible, colocate files together. It's a common practive in JavaScript projects to do so. I find it easier to navigate and understand. It's also easier to know which files are involved in one component.

@Galimede
Copy link
Member

I do find the second option to be better as well. Also it will probably be much easier to navigate through the files for us with that option than the first one.

@hsablonniere
Copy link
Member Author

About this, I know it is possible to explicitly specify the package's entry points but I had trouble using it in a previous project: nodejs.org/api/packages.html#package-entry-points

Yes, I tried experimenting with this but support with smart CDNs and some bundlers was still a bit weird... We'll try again in the future.

@hsablonniere
Copy link
Member Author

I did some experiments with rollup and I'm sad 😢

@hsablonniere
Copy link
Member Author

OK, I tried all files as inputs and it seems to work 🎉

@hsablonniere
Copy link
Member Author

hsablonniere commented Jul 21, 2022

🎉 I found a way to move files automatically with a custom config of Rollup ❤️

  • It can move all src JS files into the right place
  • It can move all stories/**/cc-component.stories.js files into the right place
  • A search and replace is necessary for the import.meta.url (we could inline the plugin an adapt it)
    • it works with a special version of the plugin
  • Test files, if we want to move them, will have to be moved manually
  • Some files in the stories folder have to be manually moved (like MD files)
  • exports are moved at the end of the files

Things that feel right:

  • components in src/components/cc-component-name/cc-component-name.js
  • stories in src/components/cc-component-name/cc-component-name.stories.js
  • smart definitions in src/components/cc-component-name/cc-component-name.smart.js
  • assets in * components in src/assets/image.svg
  • other source folders like src/lib, src/mixins, src/styles and src/translations

Things we should discuss:

├── stories
│   ├── assets
│   │   ├── 24-hours-points.js
│   │   ├── addon-plans.js
│   │   ├── country-city-points-big-orga.js
│   │   ├── country-city-points-medium-orga.js
│   │   ├── country-city-points-normal-orga.js
│   │   ├── price-system.js
│   │   └── runtime-plans.js
│   ├── atoms
│   │   └── all-form-controls.js
│   ├── lib
│   │   ├── dom.js
│   │   ├── i18n-control.js
│   │   ├── make-story.js
│   │   ├── sequence.js
│   │   ├── story-names.js
│   │   └── timers.js
│   ├── maps
│   │   └── fake-map-data.js

Those files are only used by stories.

  1. We can move them into src/stories/*
  2. We can keep them "at the root" => stories/*

NOTE: images in stories/assets were moved to src/assets.

Is it bad if we have images in there that are only used by stories?

@hsablonniere
Copy link
Member Author

Thinking about those exceptions:

  • stories/assets/image.svg going into src/assets/image.svg doesn't bother me much
    • @roberttran-cc will tell us how it could be a problem with the new icon system
  • stories/assets/*.js are mostly "fixtures", data used for stories
    • they could go into src/fixtures or src/stories/fixtures and be used by stories
    • same for stories/maps/fake-map-data.js
  • stories/atoms/all-form-controls.js is really tricky, it's a special story we reuse in other stories
  • stories/lib are story lib/helpers and I don't think we should mix them with production code lib

I propose something like this (where stories is inside src):

├── stories
│   ├── fixtures
│   │   ├── 24-hours-points.js
│   │   ├── addon-plans.js
│   │   ├── country-city-points-big-orga.js
│   │   ├── country-city-points-medium-orga.js
│   │   ├── country-city-points-normal-orga.js
│   │   ├── fake-map-data.js
│   │   ├── price-system.js
│   │   └── runtime-plans.js
│   ├── lib
│   │   ├── dom.js
│   │   ├── i18n-control.js
│   │   ├── make-story.js
│   │   ├── sequence.js
│   │   ├── story-names.js
│   │   └── timers.js
│   └── all-form-controls.js

@hsablonniere
Copy link
Member Author

NOTE: I gave a look at multiple component library repos on GitHub (thanks https://storybook.js.org/showcase/projects) and most of them put everything in src/components/ComponentName. Some of them put the component folders directly in src but I'm not sold on this (even if it means one less level).

@hsablonniere
Copy link
Member Author

NOTE: If we move the tests from test to something inside src/**, we'll have to update our ESLint config.

@hsablonniere
Copy link
Member Author

hsablonniere commented Jul 21, 2022

WARNING: I completely forgot about types.d.ts files, they must be moved and merged manually.

@hsablonniere
Copy link
Member Author

I'm considering removing most of the "folders" in the Storybook and only keep:

  • Home
  • Docs
  • Atoms
  • Molecules
  • Components

@pdesoyres-cc
Copy link
Contributor

I like the organization

├── stories
│   ├── fixtures
│   ├── lib
│   └── all-form-controls.js

I would prefer the assets used only by stories to be located inside the stories directory:

├── stories
│   ├── assets
│   ├── fixtures
│   ├── lib
│   └── all-form-controls.js

I would prefer having a components sub-directory:

├──src
│   ├── components
│   ├── stories

@hsablonniere
Copy link
Member Author

hsablonniere commented Jul 21, 2022

I like the organization

👍

I would prefer the assets used only by stories to be located inside the stories directory:

I'm OK with both 👍

I would prefer having a components sub-directory:

It's already like this 👍

@hsablonniere hsablonniere moved this from Discuss to WiP in Project board - Clever Components Jul 22, 2022
@hsablonniere hsablonniere self-assigned this Jul 22, 2022
Project board - Clever Components automation moved this from Review to Unreleased Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Something that will required a major semver release maintenance Code refactoring, project structure, dev tooling (storybook, dev server, npm tasks...)
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants