wingsuit-designsystem / wingsuit

Twig for Storybook
GNU General Public License v2.0
93 stars 16 forks source link

Upgrade Path from 1.x to 2.x #276

Open joelpittet opened 1 year ago

joelpittet commented 1 year ago

Is your feature request related to a problem? Please describe. I've been fighting security issues related to npm audit for a a while now on 1.x and wanting/hoping that some of the whoas would be mitigated by updating to 2.x of wingsuit, and hoping the upgrade path is fairly smooth. Granted that both hopes are likely naïve.

Describe the solution you'd like Clear set of steps to move from 1.x to 2.x

Describe alternatives you've considered Tried to get npm working to install wingsuit 1.x with less dependencies, since less dependencies means less to maintain and keep secure.

Additional context

christianwiedemann commented 1 year ago

Hi, we are currently working on smooth upgrade process

joelpittet commented 1 year ago

Let me know if you need any of that tested. Happy to help.

Thanks for the enormous work you've put into this!

jowan commented 8 months ago

Hi, we are currently working on smooth upgrade process

famous last words !

Also very keen and happy to test :)

christianwiedemann commented 8 months ago

Hi! There is already an upgrade path but it is not fully documented but would be great if you can test. Here are the first docs

https://github.com/wingsuit-designsystem/wingsuit/blob/release-2-0/docs/src/app/(docs)/guides/ws2-upgrade/index.mdx

jowan commented 8 months ago

ok so ... I've started. I'll try to document some of the issues here.

My environment is Ubuntu running docker with Wingsuit in a Node 18-alpine container.

My general approach was to: delete everything and get a Starterkit up and running. I started with the vanilla SCSS version but that was pretty incomplete so I moved the the Tailwind one and got it installed and spun up (issues below). I then bought in the presets and addons as required from the SCSS Starterkit and made a mixture. Once up, I bought back in a simple example from my old project and got it working, then started adding my own dependencies and fixing stuff along the way until finally adding back in everything from my project and doing a final round of fixes. So yeh, migration in a pretty loose sense. Here are some issues and notes.

Yarn Yarn would not install a few things until I upgraded it to 3. This was mostly to do with cross dependency handling. Yarn build:drupal/storybook would not run due to missing cross-env dependency, probably due to my platform.

Preset The SCSS preset uses node-sass (deprecated), I prefer dart-sass. It's also missing sourceMap: true option which caused compilation errors. (I will write my own preset though)

Migration The migration script did rename my component.stories.jsx to component.stories.wingsuit.jsx, thanks. However, in Wingsuit 1 I would have multiple component definitions per wingsuit.yml, this no longer works it seems (but, could be a different issue manifesting as this - such as my .yml structure or includes in index.js). Looking at the Tailwind Starterkit (forms) each component has a separate wingsuit.yml and .stories.wingsuit.jsx. This is a massive refactor for me. Is it possible to support this before I attempt to manually refactor everything (ergh) ? It is also going to create a lot more files. Here is a small example:

Previously, I'd have the following folder structure:

Index.js
group.stories.wingsuit.jsx
group.wingsuit.yml
component_a.twig
component_b.twig
component_c.twig
component_d.twig

I have a file: group.wingsuit.yml with the following contents, and one group.stories.wingsuit.jsx to load it.

component_a:
  use: "@group/component_a.twig"
  label: "Group/Component A"
component_b:
  use: "@group/component_b.twig"
  label: "Group/Component B"
component_c:
  use: "@group/component_c.twig"
  label: "Group/Component C"
component_d:
  use: "@group/component_d.twig"
  label: "Group/Component D"

It would correctly load and display my components in Storybook all nested under 'Group'. Now it doesn't.

  1. This displays the components incorrectly in Wingsuit, 'Component A' is taken as a parent and the others look like variants.
  2. if I try to use {{ pattern('component_d) }} it is not found.
  3. The refactor will result in the following file structure (and this is a small example), which presumably impacts on performance as its double the size.
Index.js
component_a.stories.wingsuit.jsx
component_a.wingsuit.yml
component_a.twig
component_b.stories.wingsuit.jsx
component_b.wingsuit.yml
component_b.twig
component_c.stories.wingsuit.jsx
component_c.wingsuit.yml
component_c.twig
component_d.stories.wingsuit.jsx
component_d.wingsuit.yml
component_d.twig

Question - what is the purpose of the configuration field ? as used in /source/default/patterns/03-organisms/header/header.wingsuit.yml. I can's see it doing anything, and I checked the Patters UI docs and can't see any reference to it there either. At fist glance it looked like a default variant setting, but it doesn't match up.

That being said - I am up and running, the performance increase is great. But I do have a lot of manual refactoring to do (as described above) and will probably look at a bash script to help. More to come I'm sure ....

christianwiedemann commented 8 months ago

Hi! First of all thanks for testing! To your points:

  1. cross-env > I forgot to add it to the documentation. We need this in the package.json
  2. Preset-scss: I am totaly fine with switching to dart-sass. We should also add an option.
  3. Multiple components in one wingsuit.yml is not possible in storybook 7. Each stories.wingsuit.jsx is compiled to a stories.jsx via webpack loader. And stories.jsx file can have multiple stories of one component. As a workaround I attached the other stories under the first component in the wingsuit.yml. That is the problem behind the new "grouping" behavior. So I think your refactoring is actually the right behavior. Maybe it can be done this in the upgrade process more generic. From the perfomance perspective this is much better because the each story is one entrypoint and compiles all dependencies into one chunk. So less unused components are much better. This is the main difference between wingsuit1 und wingsuit2. Wingsuit2 uses a loader to load its component and all dependend compoents. If you use a component via {{ pattern('compoenent_a') }} you need to define this component as a dependency into your main component ` hero: use: "@molecules/hero/hero.twig" label: Hero dependencies:
    • grid fields: .... `
  4. The configuration key is used to get configuration data into the pattern. It is part of ui pattern settings.

Thanks again for your testing. If you like to discuss further we can have a chat/talk on slack

jowan commented 8 months ago

Hi,

I immediately encountered the helpful Drupal-like "pattern not found, possible patterns are xxxxx" message - which is great.

I was unaware of the dependencies field so I dealt with it a different way. I made sure that component's index.js included the dependency's index.js and that that file included dependency's wingsuit file - and it worked. Like the following:

Plain card (component being viewed) folder.

/cards/plain-card
- index.js
- plain-card.stories.wingsuit.jsx
- plain-card.wingsuit.yml
- plain-card.twig

/cards/plain-card/plain-card.twig

{{ pattern('base_card') }}

/cards/plain-card/index.js

// index.js is loaded automatically.
include '.../base-card/' 

Base card (dependency) folder

/cards/base-card
- index.js
- base-card.stories.wingsuit.jsx
- base-card.wingsuit.yml
- base-card.twig

/cards/base-card/index.js

include './base-card.stories.wingsuit.jsx'

This does create a lot of dependencies the index.js, but I presume that is what is happening with the 'dependencies' field too ? and it is actually good practice to do it correctly, it just takes more thought. However - this only works/is needed if you refresh the page or if this is the first page of your session, then you must have all the dependencies listed in your index.js and the pattern will be found. If ... (as in the case above) you visited 'Base card' first, and the 'Plain card' you do not have to have the dependencies in place - they are already in memory and found. Again, I am expecting the same behavior from using the 'dependencies' field. It's simple enough though - just refresh !

Yes would like to chat - lots more questions :)

jowan commented 8 months ago

Here is the script I am using to separate out my stories (python). It is saving me a lot of time. It might at least be inspiration for someone writing in node! I am refactoring one by one, and then checking they work and deleting the originals - but you change it to be recursive and forceful.


from pathlib import Path
import oyaml as yaml
import os

# The atomic folder and the source story and wingsuit files containing the component bundle.
pattern_dir = '01-atoms/form'
story_file = 'form.stories.wingsuit.jsx'
wingsuit_file = 'form.wingsuit.yml'

base_dir = './source/default/patterns'
output_dir = os.path.join(base_dir, pattern_dir)
orig_story = os.path.join(output_dir, story_file)
orig_wingsuit = os.path.join(output_dir, wingsuit_file)

# Get the contents of the story file, this will be out template.
story_contents = Path(orig_story).read_text()

# Get the contents of the wingsuit file to separate out.
with open(orig_wingsuit, 'r') as file:

    # Convert the yaml to an orderedDict (use oyaml not pyaml)
    config = yaml.safe_load(file)

    # Loop through each item, which corresponds to a component.
    for key, value in config.items():

        # Restructure.
        new_value = {
            key: value
        }

        # Component names should have underscores, but their files should have dashes.
        file_key = key.replace('_', '-')

        # Construct new file names.
        new_story_file = file_key + '.stories.wingsuit.jsx'
        new_wingsuit_file = file_key + '.wingsuit.yml'

        # The new story is simply the template but replacing the file reference.
        new_story_contents = story_contents.replace(wingsuit_file, new_wingsuit_file)
        # The new wingsuit is simply the item we are looping through, so yaml.dump it (like json.dumps).
        new_wingsuit_contents = yaml.dump(new_value)

        # Create the paths.
        new_story = os.path.join(output_dir, new_story_file)
        new_wingsuit = os.path.join(output_dir, new_wingsuit_file)

        # Write the files.
        with open(new_story, "w") as text_file:
            text_file.write(new_story_contents)

        with open(new_wingsuit, "w") as text_file:
            text_file.write(new_wingsuit_contents)

# Maybe delete the orig files now ? hmm... maybe keep them for now, rename, check, and manually delete.
joelpittet commented 3 months ago

With a few bumps (mostly missing dependencies I believe) I managed to upgrade to 2.0!

Previously

❯ yarn audit
374 vulnerabilities found - Packages audited: 4183
Severity: 17 Low | 220 Moderate | 123 High | 14 Critical

Now:

❯ yarn audit
16 vulnerabilities found - Packages audited: 2400
Severity: 12 Moderate | 2 High | 2 Critical

Progress!

The 2 critical are:

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ critical      │ Babel vulnerable to arbitrary code execution when compiling  │
│               │ specifically crafted malicious code                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ babel-traverse                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ No patch available                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @wingsuit-designsystem/storybook                             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @wingsuit-designsystem/storybook > babel-traverse            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/1096879                     │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ critical      │ Babel vulnerable to arbitrary code execution when compiling  │
│               │ specifically crafted malicious code                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ babel-traverse                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ No patch available                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @wingsuit-designsystem/preset-drupal                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ @wingsuit-designsystem/preset-drupal >                       │
│               │ @wingsuit-designsystem/storybook > babel-traverse            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/1096879                     │