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

FEATURE: Upgrade yarn to 3.2, TS to 4.6 and replace lerna #3116

Merged
merged 50 commits into from
Sep 6, 2022

Conversation

Sebobo
Copy link
Member

@Sebobo Sebobo commented May 3, 2022

Resolves: #3115 & #3114

JamesAlias and others added 15 commits May 3, 2022 14:19
# Conflicts:
#	packages/neos-ui-backend-connector/package.json
#	packages/neos-ui-decorators/package.json
To fix:
```
error TS2742: The inferred type of 'handleTabNavItemClick' cannot be
named without a reference to
'.pnpm/@types[email protected]/node_modules/csstype'. This is likely not
portable. A type annotation is necessary.
```
# Conflicts:
#	packages/neos-ui-backend-connector/package.json
#	packages/neos-ui-contentrepository/package.json
#	packages/neos-ui-decorators/package.json
#	packages/neos-ui-extensibility/package.json
#	packages/neos-ui-i18n/package.json
#	packages/neos-ui-validators/package.json
#	packages/neos-ui/package.json
#	packages/positional-array-sorter/package.json
#	pnpm-lock.yaml
# Conflicts:
#	packages/neos-ts-interfaces/package.json
#	packages/neos-ui-backend-connector/package.json
#	packages/neos-ui-constants/package.json
#	packages/neos-ui-containers/package.json
#	packages/neos-ui-contentrepository/package.json
#	packages/neos-ui-decorators/package.json
#	packages/neos-ui-editors/package.json
#	packages/neos-ui-guest-frame/package.json
#	packages/neos-ui-i18n/package.json
#	packages/neos-ui-inspector/package.json
#	packages/neos-ui-redux-store/package.json
#	packages/neos-ui-sagas/package.json
#	packages/neos-ui/package.json
#	packages/react-proptypes/package.json
#	packages/react-ui-components/package.json
#	packages/utils-helpers/package.json
#	packages/utils-logger/package.json
#	packages/utils-redux/package.json
...and merges them with their respective `build` scripts

# Conflicts:
#	packages/neos-ui-extensibility/package.json
#	packages/positional-array-sorter/package.json
@mhsdesign
Copy link
Member

Hi, what is missing for this one?

@Sebobo
Copy link
Member Author

Sebobo commented Jul 20, 2022

As I said during our call, testing testing testing ;)

And try whether we can use the new plow-js 3.0.0

# Conflicts:
#	.circleci/config.yml
#	lerna.json
#	package.json
#	packages/babel-preset-neos-ui/package.json
#	packages/build-essentials/package.json
#	packages/debug-reason-for-rendering/package.json
#	packages/jest-preset-neos-ui/package.json
#	packages/neos-ts-interfaces/package.json
#	packages/neos-ui-backend-connector/package.json
#	packages/neos-ui-ckeditor5-bindings/package.json
#	packages/neos-ui-constants/package.json
#	packages/neos-ui-containers/package.json
#	packages/neos-ui-contentrepository/package.json
#	packages/neos-ui-decorators/package.json
#	packages/neos-ui-editors/package.json
#	packages/neos-ui-extensibility/package.json
#	packages/neos-ui-guest-frame/package.json
#	packages/neos-ui-i18n/package.json
#	packages/neos-ui-inspector/package.json
#	packages/neos-ui-redux-store/package.json
#	packages/neos-ui-sagas/package.json
#	packages/neos-ui-validators/package.json
#	packages/neos-ui-views/package.json
#	packages/neos-ui/package.json
#	packages/positional-array-sorter/package.json
#	packages/react-proptypes/package.json
#	packages/react-ui-components/package.json
#	packages/utils-logger/package.json
#	packages/utils-redux/package.json
#	yarn.lock
@markusguenther markusguenther marked this pull request as ready for review September 2, 2022 11:03
@markusguenther markusguenther added Feature Label to mark the change as feature 8.2 labels Sep 2, 2022
Copy link
Member

@markusguenther markusguenther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a change, thanks for the hard work @Sebobo @JamesAlias and @grebaldi

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is .yarn really supposed to be in the repository?

@Sebobo
Copy link
Member Author

Sebobo commented Sep 2, 2022

Is .yarn really supposed to be in the repository?

Yes. With Yarn 2 you can optionally check in quite a lot of stuff up to the point, where you don't need to install anything form a remote. Therefore achieving exactly the same build for every dev.

@kdambekalns
Copy link
Member

kdambekalns commented Sep 3, 2022

Therefore achieving exactly the same build for every dev.

  • I still firmly believe in lock files for that. The prupose of yarn.lock should be exactly this, IMHO.
  • And having things named cache in version control gives me shivers, totally.

From https://classic.yarnpkg.com/lang/en/docs/yarn-lock/:

All yarn.lock files should be checked into source control (e.g. git or mercurial). This allows Yarn to install the same exact dependency tree across all machines, whether it be your coworker’s laptop or a CI server.

And following https://classic.yarnpkg.com/en/docs/cli/install#toc-yarn-install it should be stable even for normal use to just use yarn install, but to be honest I'd prefer yarn install --frozen-lockfile over having all those files in the repository…

$ du -hs .yarn/*
155M	.yarn/cache
2.2M	.yarn/plugins
2.1M	.yarn/releases

According to https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored the cache is to be versioned for Zero-Installs, so I reckon we use that philosophy now? Not sure, since we don't have a .pnp.cjs file , so it seems we are not using "Plug'n'Play to resolve dependencies via the cache folder".

I am still a) unsure about all that and b) not entirely convinced.


Rant on.

If "the main source of entropy from the equation: Yarn itself." is what needs to be solved, then I wonder why Composer can do it. It installs the exact set of packages/version requested in the lock file. If that's not possible, it fails. To me that seems to logical and easy, that I just don't get the JS/node/… way of dependency management…

Rant off.

@mhsdesign
Copy link
Member

i agreed with the chachy thing, because it makes stuff faster no?

@Sebobo
Copy link
Member Author

Sebobo commented Sep 5, 2022

@kdambekalns during the sprint in Dresden we decided to test yarn 2 (with its new features) and pnpm in parallel and choose the one that works best for us.
Yarn 2 won due to speed, compatibility and simplicity and we also decided to test the pnp feature for faster installs and less errors which both were a problem in the past when someone wanted to work on the ui.
Yes, the lock file in yarn classic was already a big improvement in comparison to npm but it wasn't perfect.

We didn't fully switch to pnp with zero-install (therefore no pnp.cjs) yet as there are still some small issues with peer dependencies. There is also a comment in the .yarnrc.yml. We fixed most of that but not everything. But even so when you download the ui and build it, yarn will not resolve & download anything and be done very quickly (works without internet). This also makes building stages for testing much faster. pnp will speed that up further.

The 160mb are bad but we added the necessary configurations to the composer.json and .gitattributes so the cache and other yarn stuff is not included in tagged ui releases and doesn't slow down normal composer installs. I tested and verified this in another repo with the same settings.

So far none of us who worked on this branch encountered any issue with this config as to my knowledge. If anyone finds a valid reason why not to use I'm open for discussion.

Package management in the JS world is a bit of a different beast than in the happy PHP world.
And getting rid of the install step in many cases for people who want to work in the UI sounds great to me.

I'm now checking if we can finally fully switch to pnp and check the finding to give better feedback on that topic.

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for the explanation. If that's the best one can get in JS-land, then I'm not going to block it… 🤷‍♂️

@kdambekalns kdambekalns dismissed their stale review September 5, 2022 09:15

Not blocking this…

@markusguenther markusguenther merged commit 744d50c into master Sep 6, 2022
@markusguenther markusguenther deleted the feature/yarn2 branch September 6, 2022 20:26
@markusguenther
Copy link
Member

Screenshot 2022-09-07 at 04 35 40
So cool that we now have an up-to-date stack and get rid of four vulnerabilities. It looks so much better than a red flag with the four ;)

Now we can start to upgrade the rest of the dependencies :D

@mhsdesign
Copy link
Member

since this pr, the unittest wont work on ci ... see #3256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.2 Feature Label to mark the change as feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use yarn workspaces and remove lerna
7 participants