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

TASK: cleanup imports of components #3465

Merged

Conversation

crydotsnake
Copy link
Member

@crydotsnake crydotsnake commented Apr 17, 2023

Resolves: #3453

What I did

I tidied the imports of our components. So If we have more then one component, they will be imported in only one line.

How I did it

Tidied the imports of the specific components.

How to verify it
None

@crydotsnake
Copy link
Member Author

Will take a look threw the UI code again. To make sure i didnt forget something :)

@crydotsnake crydotsnake marked this pull request as draft April 17, 2023 19:03
@crydotsnake crydotsnake marked this pull request as ready for review April 17, 2023 19:25
@crydotsnake crydotsnake changed the title TASK: adjust imports of components TASK: cleanup imports of components Apr 17, 2023
@crydotsnake
Copy link
Member Author

Checked everything again.

Looks like I didn't miss anything 🙂

@JamesAlias
Copy link
Contributor

JamesAlias commented Apr 18, 2023

Does the current build system support tree-shaking? If not, this change might have an impact on bundle sizes.
cc @mhsdesign

@markusguenther
Copy link
Member

Does the current build system support tree-shaking? If not, this change might have an impact on bundle sizes. cc @mhsdesign

Tried to find out, but I think we are not using tree shaking at the moment.
The docs say that it is off by default for our case and we are not using the --tree-shaking=true flag.

@markusguenther
Copy link
Member

Ok I guess I was wrong.

By default, tree shaking is only enabled either when [bundling](https://esbuild.github.io/api/#bundle) is enabled or when the output [format](https://esbuild.github.io/api/#format) is set to iife, otherwise tree shaking is disabled. You can force-enable tree shaking by setting it to true:

The iife format is the default format unless we set the platform to node. So, we should have tree shaking at the moment. @mhsdesign is on vacation at the moment 🙈

@Sebobo
Copy link
Member

Sebobo commented Apr 18, 2023

Yes three shaking is active. The bundle would be much bigger without.

@crydotsnake
Copy link
Member Author

crydotsnake commented Apr 18, 2023

Out of interest: How exactly does this affect the way we import the components. How is the difference now compared to before? 🤔

@JamesAlias
Copy link
Contributor

Out of interest: How exactly does this affect the way we import the components. How is the difference now compared to before? 🤔

take these two lines of code:

  1. import MyComponent from 'huge-library-of-components/MyComponent'
  2. import { MyComponent } from 'huge-library-of-components'

The first line imports all the code exposed by huge-library-of-components/MyComponent.
The second line imports all the code exposed by huge-library-of-components, but only uses/assigns MyComponent.

If tree shaking is not enabled, the hole huge-library-of-component would now be in the final build.
Tree shaking removes the unused code, so it should not matter which import strategy is used.

Does this help? 🙂

@crydotsnake
Copy link
Member Author

ping

@mhsdesign
Copy link
Member

Hi hello, yes we have treeshaking turned on and it saves us roughly 0.3mb

but in this case tree-shaking should not make any difference, because we import all the react ui components either way to expose them in our api:

import * as ReactUiComponents from '@neos-project/react-ui-components';

but we should wait for #3492 as a merge conflict might turn up ^^

@crydotsnake
Copy link
Member Author

crydotsnake commented Jul 15, 2023

Since #3492 is merged and no merge conflict came up, how do we continue here?

@mhsdesign
Copy link
Member

A minor merge conflict will turn up with #3306 but if @grebaldi agrees we can merge this cleanup first ;)

  • also we should target 9.0 imo ^^

@grebaldi
Copy link
Contributor

Please go ahead :) #3306 will take some extra work anyway.

@crydotsnake
Copy link
Member Author

I would say we can merge this? @mhsdesign

@mhsdesign
Copy link
Member

good call :D Yeah but im still unsure if we should target 8.3 or 9.0?

Pro 8.3: Upmerges would be easier, if we add dependencies here

Pro 9.0: If there is a bug (which is unlikely) we are more likely to find it out fast and without any trouble as 9.0 is not released.

Maybe @markusguenther has an opinion.

But after writing this down i think we are safe to target 8.3 ;)

Copy link
Member

@jonnitto jonnitto left a comment

Choose a reason for hiding this comment

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

Fine by reading.

@markusguenther markusguenther merged commit 56732e2 into neos:8.3 Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: In progress, but not this time ...
Development

Successfully merging this pull request may close these issues.

TASK: Workspaces prefer import via main index.js instead of src/foo/bar.js
7 participants