Skip to content
This repository has been archived by the owner on Oct 22, 2020. It is now read-only.

layout has broken - everything is now left-aligned #500

Closed
jsms90 opened this issue Aug 20, 2018 · 9 comments · Fixed by #515
Closed

layout has broken - everything is now left-aligned #500

jsms90 opened this issue Aug 20, 2018 · 9 comments · Fixed by #515
Assignees
Labels
Bug Anything that does not work as expected. Client-Side Any issue regarding the look / behaviour of the application in the browser.

Comments

@jsms90
Copy link
Contributor

jsms90 commented Aug 20, 2018

Describe the bug

For some reason, everything is now left aligned

To Reproduce

Pull the latest changes from develop & start the app

Expected behavior

How the screens are supposed to look:
image
image
image

Screenshots

Now:
image
image

image

@jsms90 jsms90 added Bug Anything that does not work as expected. Client-Side Any issue regarding the look / behaviour of the application in the browser. labels Aug 20, 2018
@jsms90 jsms90 self-assigned this Aug 20, 2018
@jsms90
Copy link
Contributor Author

jsms90 commented Aug 20, 2018

My first thought was that this is a grid-styled problem, as this is what is responsible for our layout.

Looking at the recent PRs, I'm guessing the problem was introduced in this one: #484 "update dependencies"

Checking out an old commit (from a time when we definitely didn't have this layout issue) doesn't actually bring up the old, unbroken layout...until I re-run yarn. So it seems like dependency updates caused this for sure.

@diversemix
Copy link
Contributor

You got #500 🥇 ... thanks for looking into this - seems like your best placed. 👍

@jsms90
Copy link
Contributor Author

jsms90 commented Aug 20, 2018

No problem 😸

Well.. 😳 ... 🤞

So, the layout problem was introduced by this commit - "chore: update server dependencies "

Checking out the commit beforehand, running yarn and starting the app = no layout breakages.
Checking out this commit, running yarn & starting the app = layout 😢


So. Investigation.

From the commit beforehand, running yarn gives this:
image

After the "update server dependencies" commit, yarn gives this:
image

So we've got these lines, which are new:

warning " > [email protected]" has unmet peer dependency "emotion@>=9.0".
warning " > [email protected]" has unmet peer dependency "react-emotion@>=9.0"

Which is strange, since this was only supposed to be updating server dependencies. And at first, from the PR it looks like only server-side things have changed. But if you load the whole yarn.lock, you'll see that grid-styled was updated from 4.1.0 to 4.3.3

You can also see from our yarn.lock that grid-styled's depndencies have gone from styled-system ">=2.0.2" to system-components "^2.2.3".

grid-styled's CHANGELOG.md says

v4.2.0 - 2018-06-30

  • Use system-components

But the changelog doesn't mention versions 4.3.0, 4.3.1, 4.3.2 or 4.3.3.

... 🤷‍♀️

Tried changing "grid-styled": "^4.1.0", to "grid-styled": "4.1.0", in our package.json, to force it back from 4.3.3 to 4.1.0. Ran yarn. Still got our layout problems 😓

@jsms90
Copy link
Contributor Author

jsms90 commented Aug 20, 2018

Hmmm.....strangely....after checking out the commit right after that: "chore: update client side dependencies " & running yarn, the layout issues are gone.

Checking out the commit after that "update dev dependencies" & running yarn, the same thing - layout issues are still gone.

That's all 3 commits from that PR.

So now I'm doubley confused as to what's caused this. Gonna carry on checking out commits til I find another culprit

@jsms90
Copy link
Contributor Author

jsms90 commented Aug 20, 2018

Ok so it seems like this layout issue (as well as the Bad auth token problem) was introduced in PR #368 - "add uploadProgress subscription resolver"

We were fine at commit 66c00a8 - the PR to use material icons. The first commit after this point, where you can run the app - i.e. npm run server, is at 2529d70. At each of the other commits in between, the server throws various errors & the app can't be started.

image

At commit 2529d70, we have the layout problem & the Bad auth token problem. These persist in every commit since that point.

@diversemix
Copy link
Contributor

Thanks for the leg work on this @jsms90

@jsms90
Copy link
Contributor Author

jsms90 commented Aug 20, 2018

Ooof this has been a doozey!

Ok, so I've narrowed it down to a change in the commit called "update pubsweet package versions". You can see that styled-system has been updated in the yarn.lock file from version 2.3.6 to version 3.0.2.

grid-styled depends on system-components, which depends on
styled-system (version ^2.3.1)

grid-styled also depends on clean-tag, which itself depends on
styled-system (version >=2.0.0 || >=3.0.0)

For whatever reason, the addition of version 3.0.2 has broken our use of grid-styled. Looking at styled-system's CHANGELOG.md, version 3.0 seems to have made big changes

I'm unsure what the "right" way of solving this is, since it's at the level of dependencies on dependencies? Perhaps raising an issue on grid-styled? But either way, I know this is the problem for sure, because if I explicitly add "styled-system": "2.3.6" to our package.json, then our layout is back to normal 😁

@jsms90
Copy link
Contributor Author

jsms90 commented Aug 20, 2018

FYI, here's how I realised that styled-system (our of all the changes to yarn.lock in Mihail's commit) was the culprit.

At first, when I thought Tam's "chore: update server dependencies" commit had introduced the bug, because that one also broke our layout in the same way, styled-system had changed there too.

In that "chore: update server dependencies" commit, styled-system was also updated to 3.0.2.

In his next commit, which unbroke the layout again, styled-system went from 3.0.2 back down to 2.3.6. Hence the layout working again.

@jsms90
Copy link
Contributor Author

jsms90 commented Aug 21, 2018

Raised issues on styled-system (which contains clean-tag as a subfolder) & then on grid-styled itself to highlight the problem that we're having.

This morning @damnhipster and I talked about alternatives to putting styled-system directly in our package.json. Ideally, we wouldn't indicate that our app relies directly on styled-system, as this could be confusing for others down the line. Without a way to add comments to our package.json, it's not ideal to have styled-system sitting in there purely to hard-code a particular version, because of an error in grid-styled/clean-tag's own package.json.

So I tried forking clean-tag & adjusting the package.json: https://github.com/jxnblk/styled-system/pull/270/files. The idea was then to reference this fork until such time as this is fixed in clean-tag itself. I've pushed this up to a branch called fix-layout-bug - see the diff.

But this doesn't seem to work. Our layout remains broken this way. @damnhipster identified an issue in yarn: "Allow subdirectories within git repos in yarn install" yarnpkg/yarn#4725, which may be the reason - remember that clean-tag is a sub-directory inside styled-system.

After this go around, I've been starting to wonder whether this was such a useful approach anyway. This way, we would still be putting clean-tag into our package.json. The only way that this was better than my original suggestion of adding a specific version of styled-system directly in there, is that a link to my forked version of the npm package would make it more explicit that this was a temporary solution (even without the ability to write a comment into our package.json).

SO! We're back to the drawing board.

But, there is something in yarn called "resolutions" which may be good for this situation:

Why would you want to do this?

You may be depending on a package that is not updated frequently, which depends on another package that got an important upgrade. In this case, if the version range specified by your direct dependency does not cover the new sub-dependency version, you are stuck waiting for the author.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Anything that does not work as expected. Client-Side Any issue regarding the look / behaviour of the application in the browser.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants