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

#57 Dependency Improvements & Refactoring & Renaming #61

Closed
wants to merge 8 commits into from

Conversation

emircanerkul
Copy link

Improvements are done. Now all components can work as individual. All existing components are ported to this concept.

Also, there are other changes in other repositories. To be able to test and apply this PR other PR's also must be used.

emulsify-ds/emulsify-cli#173
emulsify-ds/emulsify-drupal#269

There are a couple of patterns used across all components. To share this knowledge with the team I'm also documenting it here.

image

Naming Patterns:

[component_name].component.scss: This pattern allow file to be compiled individually.
[component_name].mixin.scss: This pattern allows files to be used across all files. Do not add anything that create output. Only use for mixin or variables.
libraries.yml: Classic drupal library definition file. These file will be explored by Drupal.

Library naming should be in harmony with the file path.
atoms.links.link | atoms.text.headings | molecules.pager | molecules.menus.social | organisms.site.site_footer etc.

Usage within the Twig

Just attached what is needed. Do not attach any child dependencies. They are automatically imported when we use the same pattern.

{{ attach_library('emulsify/organisms.site.site_footer') }}

Usage within the Storybook

Styles must be included individually in the **.stories.js files.
import './site-header/_site-header.component.scss';

Further Ideas and Todos:

  • No need to use any additional system.emulsify.json file (In my opinion these just create much more complexity). We are now able to use libraries.yml files to explore all components and their dependencies. (Extracting in twig might be hard in that case we may use standard libraries.yml's dependency definitions)
  • Improve isolation in the storybook.
  • All libraries and definitions are mentioned as emulsify. Tokenization needed.

Thx!

@ModulesUnraveled
Copy link
Contributor

Wow. This is a huge rewrite. I'll take a look at this soon, but because it's so large, I'm not sure how quickly we'll be able to review it all. Thank you for your contribution!

At first glance, I'm intrigued by some of your organization recommendations, but also not sure everything follows our philosophy.

I hope to get this reviewed soon. Thanks again!

@emircanerkul
Copy link
Author

Wow. This is a huge rewrite. I'll take a look at this soon, but because it's so large, I'm not sure how quickly we'll be able to review it all. Thank you for your contribution!

At first glance, I'm intrigued by some of your organization recommendations, but also not sure everything follows our philosophy.

I hope to get this reviewed soon. Thanks again!

Yep, Good luck :) and Thank you.

I don't exactly know what emulsify's philosophy but if emulsify is a component system, component isolation/optimization/separation should be there because it's a component as it was written.

My main Idea and Point in this pr is Library separation and dependency management. We may want to create a huge website that has lots of organism variety. In Drupal, all elements/organisms/even atoms (which we might not use form atoms page to page) should not create one huge CSS file. With this dependency management, all headache is solved. Also with this technique, we have a chance to eliminate system.emulsify.json file.

Thanks to new web techs, multiple files are no problem anymore (HTTP2). But In addition to that Drupal also merge all styles that are used on the page and serves page-specific manner (according to my observations).

Historically, one of the main advantages x in having a single CSS file is the speed benefit when using HTTP1.1.
However, as of March 2018 over 80% of browsers now support HTTP2 which allows the browser to download multiple resources simultaneously as well as being able to push resources pre-emptively. Having a single CSS file for all pages means a larger than necessary file size. With proper design, I don't see any advantage in doing this other than its easier to code.

The ideal design for HTTP2 for best performance would be:

Have a core CSS file which contains common styles used across all pages.
Have page specific CSS in a separate file
Use HTTP2 push CSS to minimise wait time (a cookie can be used to prevent repeated pushes)
Optionally separate above the fold CSS and push this first and load the remaining CSS later (useful for low-bandwidth mobile devices)
You could also load remaining CSS for the site or specific pages after the page has loaded if you want to speed up future page loads.

https://stackoverflow.com/a/49397312

@ModulesUnraveled
Copy link
Contributor

Are you available to join our community meetup at any time? We meet on the first Thursday of each month at 1pm Central (I think that's 9pm in Turkey).

One of my concerns with this is that it's very Drupal-specific. While we do a lot of Drupal work, our Components support Wordpress, and potentially other frameworks. So, we try to keep anything Drupal-specific (like libraries.yml files) out of Compound etc.

One other thing is that the system.emulsify.json file is built to support our CLI which enables organizations to share a common component library across projects. You can easily install an organization component into each project as needed. So, we wouldn't want to get rid of that, because we'd lose some vital functionality.

Anyway, we have an Emulsify Slack (which you can join here: https://launchpass.com/emulsify) and we meet once a month as a community. If you could come to one of those, I'd love to get a walk-through of the ideas you've presented here, and see if there are parts that we want to include in "core".

@emircanerkul
Copy link
Author

emircanerkul commented Sep 30, 2022

@ModulesUnraveled yes sure.

Firstly I want to clear some points. I am aware of cli and its advantage. I do not want to remove this functionality. It's really useful.

Let's think first about why we need the system.emulsify.json file. It's obvious to say give information to emulsify cli to perform automatic imports. So after the individual libraries.yml file approach, we can grab the same information from these files, and under the hood, we can dynamically build this system.emulsify.json file. With this approach, we are just trying to eliminate duplicate work.

My other point; YAML files are not especially related to drupal. YAML files are just little files that contain component information individually in this approach.

We can change file names even if we also can use a JSON file. But I selected the YAML file because the helper function is already developed for drupal and drupal's libraries discovery work with YAML.

I think All bricks come together. If any more are left we also can discuss them in slack.

@ModulesUnraveled
Copy link
Contributor

@emircanerkul Thanks again for this, there's a lot of work in here that we really like.

However, it's such a large PR that it's hard to review all of the parts at once. We discussed this in our weekly checkin, and would like to ask if you can break this into separate PRs that are more focused on incremental changes, rather than globbing it all into one giant PR.

For example:

  • Splitting the Mixins out into their own *.mixin.scss files makes a lot of sense, and could be its own PR.
  • The way you're creating individual libraries files is still a little fuzzy to me, so having that work in an isolated PR would help me to separate the work related to that from the css updates, for example

@emircanerkul
Copy link
Author

@ModulesUnraveled I'm unsure if I can split to multiple PRs because all depend on each other, including PRs that I sent in other repos. But lots of parts in this pr follow a similar pattern.

Actually, individual libraries are the main point of this PR. To create a dependency chain, it's required for webpack and drupal. All component's assets are compiled and included as individuals. The Drupal library system grabs each required one and optimizes the output. Only required component's assets are included on the drupal page.

image

@emircanerkul
Copy link
Author

Anyway, I am also not a fan of twig. Currently, I am involved in js technologies these days. SvelteKit is just a really good one. With vite it's really speedy and lots of capabilities they have. All optimization and hassle-free configs are really great to work with unlike webpack and twig and library dependency things...

Might be my work in this repo is for extreme cases and performance and optimization things. But now vite and sveltekit's component system and encapsulation are already placed and works perfect.

I think in drupal we just need to make entity management (structural content) and API's perfect (Just run as a headless one). Otherwise, we can't take any advantage of other works in open source (huge js ecosystem especially) and it seems not wise to me.

BTW, feel free to close my all PRs. Without dependency separation, this also works. And on these days I think no one cares about an extra 10kb in the assets file. Let's say 2 MB in a huge project. It's still not a big deal.

Thank you for all.

@josue2591
Copy link
Contributor

@callinmullaney due to the phase we are with this project I think we can close this PR

@josue2591 josue2591 added ❓ Closed If the PR can be close and removed 🚫 On Hold labels Mar 15, 2024
@josue2591 josue2591 closed this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❓ Closed If the PR can be close
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants