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

[work in progress] new homepage #916

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

Connoropolous
Copy link
Member

No description provided.

@@ -10,4 +10,6 @@
# Precompile additional assets.
# application.js, application.css, and all non-JS/CSS in app/assets folder are already added.
config.assets.precompile += %w(webpacked/metamaps.bundle.js)
config.assets.precompile += %w(webpacked/homepage.js)
config.assets.precompile += %w(home.css)
Copy link
Contributor

Choose a reason for hiding this comment

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

these three can be

config.assets.precompile += %w(webpacked/metamaps.bundle.js homepage.js home.css)

Copy link
Member Author

Choose a reason for hiding this comment

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

cool

@@ -35,7 +35,8 @@ const config = module.exports = {
]
},
entry: {
'metamaps.bundle': './frontend/src/index.js'
'metamaps.bundle': './frontend/src/index.js',
'homepage': './frontend/src/homepage.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

it's probably better to include this code in the main bundle

Let's say we want to use the react package in the homepage code (not unreasonable). Now react will be loaded twice; once in the main bundle and once in this bundle, and that causes big bugs with react.

we could make this into a function, either in a window.Metamaps.Homepage.init() option, or just store it in window.Homepage.init()

Copy link
Member Author

Choose a reason for hiding this comment

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

The homepage is actually loading it's entirely own thing. The plan is to not include the main bundle and all its code on the homepage

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm still uneasy about this - i think it'll be better for us to use one bundle.

There's a webpack feature called code splitting that handles this setup much more gracefully than having multiple entry points/bundles. It's harder to set up but I think it's better long term.

I guess I'm OK leaving this as is for now, but we have to be really vigilant about this or it could cause weird bugs if a library is loaded twice (including a library that's a dependency of other libraries, so we can't just check by looking at package.json).

Copy link
Member Author

Choose a reason for hiding this comment

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

really happy to do it another way, I just wouldn't know how. Not familiar with webpack at all really

Copy link
Contributor

Choose a reason for hiding this comment

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

if i had my way, you'd just move this into one bundle.

if you want to make the homepage load faster NOW, then leave it as is. But before merge can we use npm ls or something to figure out if any dependencies overlap? Well-designed modules won't overlap because webpack has module scope, but something like React will break if it's loaded twice (that's what's burned me before).

Code-splitting: I also don't understand it, but it's on my roadmap. Maybe I'll learn it at my new job and can contribute back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants