Bugfix/fixing storybook venia ui #1945

LucasCalazans · 2019-10-30T05:45:38Z

Description

Venia UI was not able to execute Storybook (Probably since the separation with venia-concept) because it was depending on a lot of the venia-concept files.

My solution was to split the main part of the configureWebpack data in the base.webpack.config.js file, with that we can import the same file/config into the venia-concept and venia-ui without duplicate the code.

To do that I had to include @magento/pwa-buildpack as a peerDependency of venia-ui. Without that, we, probably, would have to import the webpack config manually.

Related Issue

Closes #1577.

Acceptance

Verification Stakeholders

Specification

Verification Steps

Go to the root project folder.
Run yarn storybook:venia

OR

Go to the venia-ui package folder
RUN yarn storybook

Checklist

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

Additional Information

I fixed some problems related to Context API (Some components are using) adding a storybook decorator. With that, we will no longer need to add the Adapter neither the AppContextProvider.

Problems

We don't have the index.css anymore so some parts are missing styles (Like the MiniCart component)
The cartTrigger component is calling REST APIs without using the BACKEND_ENDPOINT, so the calls are been succeded.

PWAStudioBot · 2019-10-30T05:50:44Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against e224820

zetlen · 2019-10-31T21:49:46Z

@LucasCalazans Thank you for working on this abstraction. It's very well thought out; however, I'm not sure we want to add another layer of default Webpack config beyond configureWebpack at this stage. The configureWebpack interface is subject to future changes to make it compose together the needs of various dependent libraries, so in the future we shouldn't need this.

I don't really have a problem with implicit dev dependencies between venia-ui and venia-concept. Because venia-ui and venia-concept are developed only in this repo, the venia-ui storybook should only run if you have the full repo checked out (or you have deployed it in a custom way, as in our documentation). So I'm wondering if we could fix #1577 simply by changing the imported Webpack path to import from the correct package.

Maybe I'm forgetting something that @supernova-at could remind me of; this bug is pretty old, so maybe it's more complicated than that.

supernova-at · 2019-11-05T15:49:42Z

Yes!!! Thank you so much for this. Taking a look now.

supernova-at · 2019-11-05T15:52:40Z

we, probably, would have to import the webpack config manually

I don't really have a problem with implicit dev dependencies between venia-ui and venia-concept. Because venia-ui and venia-concept are developed only in this repo, the venia-ui storybook should only run if you have the full repo checked out (or you have deployed it in a custom way, as in our documentation). So I'm wondering if we could fix #1577 simply by changing the imported Webpack path to import from the correct package.

Agreed, let's try importing it manually.

I'd love to get index.css back, and some new Image stories just landed that it looks like GitHub is saying have some conflicts.

LucasCalazans · 2019-11-07T22:06:04Z

@zetlen I see. I was thinking about an application that just uses the venia-ui without the venia-concept.

But yeah, sorry about the delay in the answer, I'm working in a new version based on the venia-concept dev dependency. I'll try to push a commit today

LucasCalazans · 2019-11-07T22:08:41Z

@supernova-at Just to be clear, do you think that we should import the webpack dependencies in the venia-ui or import the venia-concept's webpack.config?

…lazans/pwa-studio into bugfix/fixing-storybook-venia-ui

LucasCalazans · 2019-11-07T23:13:48Z

Hey guys, I've tried importing the venia-concept, do you agree with this? Can you provide a new review please?

It's now getting some errors in the new Image stories. I've debugged and it seems to be a problem with some params that were not passed into the Image component like the resource that is required but in some stories (like 'An Image without a placeholder (throttle me)') it is not passed.

packages/venia-ui/.storybook/config.js

supernova-at · 2019-11-11T19:58:42Z

I think this is great - definitely love moving the Adapter into the setup area and out of individual stories 💯

yarn storybook:venia isn't working for me, though. For some reason it's building the storybook-dist directory but it doesn't actually launch storybook.

I "fixed it" by updating the root package.json scripts:
"storybook:venia": "pushd ./packages/venia-ui && yarn storybook && popd",

If you can figure out why it's building Storybook instead of running it, I'd prefer that.

I can verify that Storybook starts up without error, even though many of the stories are broken.
I'm okay with creating follow up tickets to address the actual contents of our Storybook.

supernova-at · 2019-11-11T20:22:50Z

Also I see A new version (5.2.6) is available!, do we want to upgrade Storybook as part of this PR?

package.json

packages/venia-ui/.storybook/config.js

sirugh · 2019-11-25T16:08:13Z

packages/venia-ui/.storybook/getEnvironmentVariable.js

+ * Returns the value of the first variable in any section whose name matches the input.
+ * @param {string} targetVariable - The name of the environment variable to find.
+ */
+const getEnvironmentVariable = targetVariable => {


Can you just use loadEnvironment instead of this custom function? loadEnvironment should be able to do exactly what this function is doing.

Hmmm, actually I'm not sure

I tried to use it but it seems to have a dependency exactly where I need, in the MAGENTO_BACKEND_URL, when I use loadEnvironment to get this variable it throws a exception:

ⓧ Missing required environment variables: MAGENTO_BACKEND_URL: Connect to an instance of Magento 2.3 by specifying its public domain name. (eg. "https://master-7rqtwti-mfwmkrjfqvbjk.us-4.magentosite.cloud/")

I tried using this code:

const loadEnvironment = require('@magento/pwa-buildpack/lib/Utilities/loadEnvironment'); loadEnvironment('MAGENTO_BACKEND_URL')

Do you know why is this happening? Maybe I'm using it in a wrong way

I'm not sure about this either. The function signature of loadEnvironment is

function loadEnvironment(dirOrEnv, logger = prettyLogger) {

so it doesn't look like it wants the name of the environment variable.

@LucasCalazans check out other places where we use it. For example in create-custom-origin:

const projectConfig = loadEnvironment(projectRoot); if (projectConfig.error) { failExpected(projectConfig.error); } const config = projectConfig.section('customOrigin'); // etc

You pass it a directory and it returns an object with props on it. I'm not sure how well it is documented unfortunately. It may take some experimenting.

sirugh · 2019-11-25T16:14:37Z

packages/venia-ui/package.json

@@ -75,6 +75,8 @@
  "peerDependencies": {
    "@magento/babel-preset-peregrine": "~1.0.0",
    "@magento/peregrine": "~4.0.0",
+    "@magento/pwa-buildpack": "~4.0.0",
+    "@magento/venia-concept": "~4.0.0",


The idea is that peregrine is used by venia-ui is used by venia-concept. This seems like a red flag to me? But maybe I'm misunderstanding peer deps :P

I think this, along with the imports of venia-concept files in venia-ui just seem like red flags to me in terms of where we are supposed to be keeping these things. Maybe in the future we will move the majority of the base config to the top-level and venia-concept and venia-ui/storybook can just import/use it.

TL;DR - I think I'm fine with this, just rambling.

Yeah I think for the storefront app that's correct. For Storybook we relaxed the rules a bit so we could run it easily.

Yeah, it's a tough call. I do think @sirugh has the right of it here.

It's true that venia-ui components are hard to render without the glue that venia-concept adds, so we need to get that glue from somewhere. But venia-ui is built to be a dependency of venia-concept, so I'd prefer that it didn't list venia-concept as a dependency or import anything from it. Thinking about our various options here:

Option Evaluation

Duplicate store.js, webpack.config.js, and index.css bad

Move these files up to some shared package bad

Import these files from venia-concept not ideal

Move venia-ui storybook stories into venia-concept okay?

Overall, I would accept the approach in this PR because the work's been put in, it has value, and only the Storybook files are importing from venia-concept. But eventually I want to remove the circular dependency.

revanth0212

Thanks for making the changes. I have not tested the PR for functionality yet. Will that soon. From code POV, it is good.

sirugh · 2019-12-02T16:58:21Z

@LucasCalazans Are you working on the last changes I requested? Just checking :)

sirugh · 2019-12-03T16:02:56Z

I have some ideas on how to improve this. I'll go ahead and make them, should be small changes.

…concept

sirugh · 2019-12-06T02:24:00Z

packages/venia-ui/.storybook/webpack.config.js

@@ -6,7 +6,7 @@ const {
 const baseWebpackConfig = require('@magento/venia-concept/webpack.config');
 const { DefinePlugin, EnvironmentPlugin } = require('webpack');

-module.exports = async storybookBaseConfig => {
+module.exports = async ({ config: storybookBaseConfig }) => {


LucasCalazans added 2 commits October 30, 2019 02:26

Creating a webpack base configuration in the buildpack package

60445b4

Executing prettier

027ccff

magento-engcom-team added Partner: Webjump partners-contribution labels Oct 30, 2019

PWAStudioBot added pkg:pwa-buildpack pkg:venia-concept pkg:venia-ui labels Oct 30, 2019

Merge branch 'develop' into bugfix/fixing-storybook-venia-ui

4f0c509

supernova-at self-assigned this Nov 5, 2019

m2-community-project bot added the Progress: review label Nov 5, 2019

m2-community-project bot added Progress: changes requested and removed Progress: review labels Nov 5, 2019

LucasCalazans added 4 commits November 7, 2019 19:27

Importing data from venia-concept

a68b274

Merge branch 'bugfix/fixing-storybook-venia-ui' of github.com:LucasCa…

903702b

…lazans/pwa-studio into bugfix/fixing-storybook-venia-ui

Resolving conflicts with develop

a8826c9

Resolving conflicts part 2

ff4d83b

supernova-at reviewed Nov 11, 2019

View reviewed changes

packages/venia-ui/.storybook/config.js Show resolved Hide resolved

Merge branch 'develop' into bugfix/fixing-storybook-venia-ui

749acff

sirugh requested changes Nov 25, 2019

View reviewed changes

m2-community-project bot assigned sirugh Nov 25, 2019

Using yarn workspace to storybook script

62de623

LucasCalazans dismissed supernova-at’s stale review via 62de623 November 26, 2019 13:46

LucasCalazans and others added 2 commits November 26, 2019 10:47

Merge branch 'develop' into bugfix/fixing-storybook-venia-ui

cb4526b

Merge branch 'develop' into bugfix/fixing-storybook-venia-ui

d195d5e

revanth0212 previously approved these changes Nov 27, 2019

View reviewed changes

Use loadEnvironment to obtain references to env variables from venia-…

1e0a4f0

…concept

sirugh dismissed revanth0212’s stale review via 1e0a4f0 December 3, 2019 17:55

m2-community-project bot assigned jimbo Dec 3, 2019

m2-community-project bot added Progress: review and removed Progress: changes requested labels Dec 3, 2019

Merge branch 'develop' into bugfix/fixing-storybook-venia-ui

9eca071

supernova-at previously approved these changes Dec 4, 2019

View reviewed changes

tjwiebell added 2 commits December 5, 2019 17:14

Merge remote-tracking branch 'origin/develop' into pr/LucasCalazans/1945

ae9f529

Change function signature back

b37839d

tjwiebell dismissed supernova-at’s stale review via b37839d December 6, 2019 00:32

sirugh reviewed Dec 6, 2019

View reviewed changes

Add comments and mode

e224820

tjwiebell approved these changes Dec 6, 2019

View reviewed changes

m2-community-project bot assigned tjwiebell Dec 6, 2019

m2-community-project bot added Progress: approved and removed Progress: review labels Dec 6, 2019

tjwiebell merged commit a5bbb1c into magento:develop Dec 6, 2019

m2-community-project bot removed the Progress: approved label Dec 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/fixing storybook venia ui #1945

Bugfix/fixing storybook venia ui #1945

LucasCalazans commented Oct 30, 2019

PWAStudioBot commented Oct 30, 2019 •

edited

Loading

zetlen commented Oct 31, 2019

supernova-at commented Nov 5, 2019

supernova-at commented Nov 5, 2019

LucasCalazans commented Nov 7, 2019

LucasCalazans commented Nov 7, 2019

LucasCalazans commented Nov 7, 2019

supernova-at commented Nov 11, 2019 •

edited

Loading

supernova-at commented Nov 11, 2019

sirugh Nov 25, 2019

LucasCalazans Nov 26, 2019

supernova-at Nov 26, 2019

sirugh Nov 26, 2019

sirugh Nov 25, 2019

supernova-at Nov 26, 2019

jimbo Dec 3, 2019

revanth0212 left a comment

sirugh commented Dec 2, 2019

sirugh commented Dec 3, 2019

sirugh Dec 6, 2019

Option	Evaluation
Duplicate `store.js`, `webpack.config.js`, and `index.css`	bad
Move these files up to some shared package	bad
Import these files from `venia-concept`	not ideal
Move `venia-ui` storybook stories into `venia-concept`	okay?

Bugfix/fixing storybook venia ui #1945

Bugfix/fixing storybook venia ui #1945

Conversation

LucasCalazans commented Oct 30, 2019

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Checklist

Additional Information

Problems

PWAStudioBot commented Oct 30, 2019 • edited Loading

zetlen commented Oct 31, 2019

supernova-at commented Nov 5, 2019

supernova-at commented Nov 5, 2019

LucasCalazans commented Nov 7, 2019

LucasCalazans commented Nov 7, 2019

LucasCalazans commented Nov 7, 2019

supernova-at commented Nov 11, 2019 • edited Loading

supernova-at commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revanth0212 left a comment

Choose a reason for hiding this comment

sirugh commented Dec 2, 2019

sirugh commented Dec 3, 2019

Choose a reason for hiding this comment

PWAStudioBot commented Oct 30, 2019 •

edited

Loading

supernova-at commented Nov 11, 2019 •

edited

Loading