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

Add pre and postinstall script to fix custom build with ckeditor and custom builds on windows #96

Merged
merged 22 commits into from
Mar 10, 2021

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Mar 3, 2021

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets fixes #issuenum
Related issues/PRs #issuenum
License MIT
Documentation PR sulu/sulu-docs#prnum

What's in this PR?

Add symlink for dependencies to workaround a behaviour npm does when thinking a dependencies is not a child of the current directory.

Why?

Npm can not work well when a depenencies is not a children.

Sideffects (resolved: see beneath)

Before:

drwxr-xr-x   8 alexanderschranz  staff       256  3 Mär 16:33 ./
drwxr-xr-x   3 alexanderschranz  staff        96  3 Mär 16:33 ../
drwxr-xr-x  60 alexanderschranz  staff      1920  3 Mär 16:33 fonts/
drwxr-xr-x  10 alexanderschranz  staff       320  3 Mär 16:33 images/
-rw-r--r--   1 alexanderschranz  staff    205585  3 Mär 16:33 main.7d43ff714b27ee1811ea.css
-rw-r--r--   1 alexanderschranz  staff   2748476  3 Mär 16:33 main.7d43ff714b27ee1811ea.js
-rw-r--r--   1 alexanderschranz  staff  11210456  3 Mär 16:33 main.7d43ff714b27ee1811ea.js.map
-rw-r--r--   1 alexanderschranz  staff      1179  3 Mär 16:33 manifest.json

After:

drwxr-xr-x   8 alexanderschranz  staff       256  3 Mär 16:46 ./
drwxr-xr-x   3 alexanderschranz  staff        96  3 Mär 16:46 ../
drwxr-xr-x  60 alexanderschranz  staff      1920  3 Mär 16:46 fonts/
drwxr-xr-x  10 alexanderschranz  staff       320  3 Mär 16:46 images/
-rw-r--r--   1 alexanderschranz  staff    205585  3 Mär 16:46 main.f8de9ff585c5662613ea.css
-rw-r--r--   1 alexanderschranz  staff   3233821  3 Mär 16:46 main.f8de9ff585c5662613ea.js
-rw-r--r--   1 alexanderschranz  staff  10846655  3 Mär 16:46 main.f8de9ff585c5662613ea.js.map
-rw-r--r--   1 alexanderschranz  staff      1179  3 Mär 16:46 manifest.json

The new build is about ~3,1 and the old one was just ~2,6M. Strange is that there are less duplicated dependencies as before: https://www.diffchecker.com/Ge6npC1O still the build is bigger. Here a diff of the sizes stats: https://www.diffchecker.com/jrRrou54. As analysed the ckeditor package size has a big difference now: https://www.diffchecker.com/PM8Fk5F0

After adding .browserlistrc

drwxr-xr-x   8 alexanderschranz  staff       256  9 Mär 13:43 ./
drwxr-xr-x   3 alexanderschranz  staff        96  9 Mär 13:43 ../
drwxr-xr-x  60 alexanderschranz  staff      1920  9 Mär 13:43 fonts/
drwxr-xr-x  10 alexanderschranz  staff       320  9 Mär 13:43 images/
-rw-r--r--   1 alexanderschranz  staff    187511  9 Mär 13:43 main.299ab210e717d74ee262.css
-rw-r--r--   1 alexanderschranz  staff   2556246  9 Mär 13:43 main.299ab210e717d74ee262.js
-rw-r--r--   1 alexanderschranz  staff  10287505  9 Mär 13:43 main.299ab210e717d74ee262.js.map
-rw-r--r--   1 alexanderschranz  staff      1180  9 Mär 13:43 manifest.json

Its about ~2.4mb.

@alexander-schranz alexander-schranz changed the base branch from 2.x to 2.2 March 3, 2021 15:51
@alexander-schranz alexander-schranz marked this pull request as draft March 3, 2021 15:51
@alexander-schranz alexander-schranz force-pushed the feature/link-dependencies branch from 125a82d to ff6d9f1 Compare March 4, 2021 14:33
@niklasnatter niklasnatter changed the base branch from 2.2 to 2.1 March 8, 2021 09:38
composer.json Outdated Show resolved Hide resolved
@alexander-schranz alexander-schranz force-pushed the feature/link-dependencies branch from 72bc866 to e8274d6 Compare March 8, 2021 10:56
@alexander-schranz alexander-schranz force-pushed the feature/link-dependencies branch from 993fd35 to 3e69159 Compare March 8, 2021 15:04
Co-authored-by: nnatter <niklas.natter@gmail.com>
@alexander-schranz alexander-schranz changed the title WIP: Add symlink for dependencies Add pre and postinstall script to fix build with npm 7 and on windows Mar 9, 2021
@@ -35,7 +35,7 @@
"friendsofsymfony/http-cache-bundle": "^2.8",
"handcraftedinthealps/zendsearch": "^2.0",
"jackalope/jackalope-doctrine-dbal": "^1.3",
"sulu/sulu": "~2.1.9",
"sulu/sulu": "~2.1.9 || 2.1.*@dev",
Copy link
Contributor

Choose a reason for hiding this comment

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

how should we handle this? i guess we need to remember to reset this before releasing the next version? 🤔

Copy link
Member Author

@alexander-schranz alexander-schranz Mar 9, 2021

Choose a reason for hiding this comment

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

on release this always should be set to the correct version: ~2.1.10

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good - hope we remember that 🙂

- php-version: '7.4'
mysql-version: '8.0'
phpcr-transport: 'jackrabbit'
dependency-versions: 'highest'
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if we will test anything else here 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

IDK

Copy link
Member Author

Choose a reason for hiding this comment

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

highest is I think always the default for composer but maybe it has an impact if a lock is commitet or not to this github action. but for our case I think it doesn't matter and would keep as it is

@alexander-schranz alexander-schranz force-pushed the feature/link-dependencies branch from aa6be5c to e4665ab Compare March 9, 2021 15:34
@alexander-schranz alexander-schranz marked this pull request as ready for review March 9, 2021 15:34
@alexander-schranz alexander-schranz added the bug Something isn't working label Mar 9, 2021
@niklasnatter niklasnatter merged commit 3fa208a into sulu:2.1 Mar 10, 2021
@alexander-schranz alexander-schranz deleted the feature/link-dependencies branch March 19, 2021 09:26
@alexander-schranz alexander-schranz changed the title Add pre and postinstall script to fix build with npm 7 and on windows Add pre and postinstall script to fix custom build with ckeditor and custom builds on windows Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants