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

Remove production build resource filtering behaviour #262

Open
locks opened this issue Jun 11, 2020 · 14 comments
Open

Remove production build resource filtering behaviour #262

locks opened this issue Jun 11, 2020 · 14 comments
Assignees

Comments

@locks
Copy link
Collaborator

locks commented Jun 11, 2020

At the moment the addon removes all of the resources from the build when doing a production build. Given that at the moment the default npm build command is a production build, it means that the default deploy experience is a broken app, since it will try to render a non-existent <WelcomePage> component.

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2020

I don't particularly think this is a good solution. Shipping needless bytes to all users that don't manually audit their dependencies/devDependencies seems like a mega-trolly thing to do.

@locks
Copy link
Collaborator Author

locks commented Jun 16, 2020

@rwjblue I understand and that was the original impetus behind the current implementation. I feel like --no-welcome is widely used, and we have gotten feedback that a broken build out of the box is giga-trolly, hence the desire to move the compromise in the direction of shipping the bytes.
I just did a check so we have some data:

Welcome:

ember-with-welcome on  master via ⬢ v12.18.0
❯ npm run build

> [email protected] build /Users/ricardo/src/ember-with-welcome
> ember build --environment=production

Environment: production
cleaning up...
Built project successfully. Stored in "dist/".
File sizes:
 - dist/assets/auto-import-fastboot-d41d8cd98f00b204e9800998ecf8427e.js: 0 B
 - dist/assets/ember-with-welcome-d41d8cd98f00b204e9800998ecf8427e.css: 0 B
 - dist/assets/ember-with-welcome-f4584c65790efbc69abe9a5e41fc674e.js: 10.79 KB (2.32 KB gzipped)
 - dist/assets/vendor-1c3e6abe7cbf6750ad2aee115997b297.js: 715.56 KB (183.79 KB gzipped)
 - dist/assets/vendor-d41d8cd98f00b204e9800998ecf8427e.css: 0 B

No welcome:

ember-without-welcome on  master via ⬢ v12.18.0
❯ npm run build

> [email protected] build /Users/ricardo/src/ember-without-welcome
> ember build --environment=production

Environment: production
cleaning up...
Built project successfully. Stored in "dist/".
File sizes:
 - dist/assets/auto-import-fastboot-d41d8cd98f00b204e9800998ecf8427e.js: 0 B
 - dist/assets/ember-without-welcome-d41d8cd98f00b204e9800998ecf8427e.css: 0 B
 - dist/assets/ember-without-welcome-f3863e6c4dfa71cbf88a7ed4f7f00aec.js: 10.9 KB (2.34 KB gzipped)
 - dist/assets/vendor-1c3e6abe7cbf6750ad2aee115997b297.js: 715.56 KB (183.79 KB gzipped)
 - dist/assets/vendor-d41d8cd98f00b204e9800998ecf8427e.css: 0 B

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2020

Why not just have folks add welcome in the tutorial?

@acorncom
Copy link
Member

Because welcome is aimed more at folks who just setup a new Ember app who may or may not have followed the tutorial

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2020

Because welcome is aimed more at folks who just setup a new Ember app who may or may not have followed the tutorial

Sure, and those folks aren't doing deployments...

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2020

Basically, the proposed change negatively impacts all users that haven't manually removed this addon. Whereas the current implementation seems to negatively impact a much smaller subset (those that ember new foo and attempt deployment without actually authoring anything in their templates).

@acorncom
Copy link
Member

Sure, and those folks aren't doing deployments...

That's what's been so surprising ... 😄 There seem to be a good number of long-time Ember folks who fire up a brand new app, make sure it's running smoothly in dev and then push a deploy to make sure everything is working. And then are majorly trolled by the fact that "this was just working locally, what is going on in production?"

I'm ambivalent on the change, so will step out at this point, but that's the context that I've seen repeatedly here. But I agree with you on the other concern, it's why you and I built it the way it's built in the first place 😉

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2020

For example, basically everyone of these addons that have a demo app would now be shipping extra code:

https://emberobserver.com/code-search?codeQuery=ember-welcome-page&fileFilter=package.json

Since --welcome defaults to enabled, this proposal is very likely to troll the inverse (and therefore large IMHO) set of folks.

@rwjblue
Copy link
Member

rwjblue commented Jun 16, 2020

There seem to be a good number of long-time Ember folks who fire up a brand new app, make sure it's running smoothly in dev and then push a deploy to make sure everything is working. And then are majorly trolled by the fact that "this was just working locally, what is going on in production?"

This can be fixed a number of ways though, that doesn't also negatively impact the larger set of users.

For example:

  • Change the default of --welcome to false, so you'd have to opt-in to it
  • Remove the addon and app code from here, and just generate those components in the app instead (leaving ember-welcome-page largely as a place to host the assets)
  • Change the addon to error on a production build (prevents the error condition explicitly mentioned)

@RobbieTheWagner
Copy link
Contributor

I would be in favor of removing welcome by default or possibly moving the contents into the actual app that is generated. I believe other frameworks tend to have their default pages built inside the app, rather than from an external package.

@RobbieTheWagner
Copy link
Contributor

How do we come to a resolution here?

@RobbieTheWagner
Copy link
Contributor

@locks @rwjblue should we discuss this further? Would be good to move forward here one way or another.

@RobbieTheWagner
Copy link
Contributor

@locks @rwjblue can you guys please hash this out, so we can move on with this effort?

@MelSumner
Copy link
Contributor

I think we should leave it like it is now.

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 a pull request may close this issue.

5 participants