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

[1.7.0.1] Optionally disable web.browser/web.browser.legacy in development #9948

Closed
veered opened this issue Jun 5, 2018 · 22 comments
Closed

[1.7.0.1] Optionally disable web.browser/web.browser.legacy in development #9948

veered opened this issue Jun 5, 2018 · 22 comments
Labels
Milestone

Comments

@veered
Copy link
Contributor

@veered veered commented Jun 5, 2018

The overhead of compiling an additional client bundle on every code reload is quite large for big projects. It would be very helpful if it were possible to configure which client bundles are built.

The most likely case for development is wanting to disable web.browser.legacy. If the minimum browser requirements set by modern-browsers were set to require a modern browser, perhaps the web.browser.legacy bundle could just use the web.browser bundle. They would be identical anyways.

@benjamn
Copy link
Member

@benjamn benjamn commented Jun 6, 2018

That's not quite how the minimum browser requirements work. Additional calls to setMinimumBrowserVersions can only restrict the set of browsers/versions that are considered modern, which enlarges the set of legacy browsers (i.e., all the ones that aren't modern). In other words, there's no way to relax the requirements so that every browser will be considered modern. That wouldn't be safe anyways, since not every browser is modern by any reasonable definition.

If rebuild times were quicker, the complexity of somehow disabling the web.browser.legacy build (and then at some point reenabling it… but when?) would matter a lot less. I would rather use the motivation behind this issue to improve rebuild times generally. Skipping an important part of the build process feels like cheating, to me.

In order to move forward with this feature request, I think we need

  • a mechanism for skipping web.browser.legacy builds that doesn't become permanent because the developer forgets to turn it off
  • some mechanism for reporting legacy build errors reliably even though we aren't building the web.browser.legacy bundle as often
  • a decision about what should happen when/if the developer visits localhost:3000 using a legacy browser
@benjamn
Copy link
Member

@benjamn benjamn commented Jun 6, 2018

To be clear, I'm sympathetic to the motivation here, I just haven't been able to come up with a developer workflow that feels right / gets all the details right. Zero-configuration may not be possible for something like this, so we have to be really thoughtful about how this behavior is configured (and reconfigured over time).

@spencern
Copy link

@spencern spencern commented Jun 6, 2018

This was a use-case I thought of immediately when watching your talk on 1.7 @benjamn.

I'd be super happy to have a flag of some kind that would permit skipping web.browser.legacy builds in development even if legacy build errors were not reported until web.browser.legacy builds were re-enabled.

As for what should happen when a developer visits localhost:3000 using a legacy browser after setting a flag to not include them in the build? If the site renders poorly or not-at all, that's good enough for me. Even better would be to log an error to the console reminding the developer they have disabled that build.

@Floriferous
Copy link
Member

@Floriferous Floriferous commented Jun 6, 2018

I ran a few benchmarks, as this is the first thing that caught my attention when upgrading:

Here's a few attempts at changing the same pure-client-side line of javascript 3 times in a row.

  • Meteor 1.6:
    • Attempt 1: 4239ms
    • Attempt 2: 2991ms
    • Attempt 3: 2443ms
  • Meteor 1.7
    • Attempt 1: 5271ms
    • Attempt 2: 3551ms
    • Attempt 3: 3087ms

Interestingly, the ProjectContext prepareProjectForBuild step was reduced from ~550ms to ~80ms in Meteor 1.7, sadly it appears those savings were offset by the new additional legacy rebuild times.

That's an increase of roughly 19-26% in reload times.

Meteor's rebuild times is probably the most frustrating part of building meteor apps to me, I always feel like I go back to the stone-age when I've spent a couple days hacking on a webpack project with hot reloads, and suddenly every change on my meteor app takes 3-10 seconds (depending on if you do a server restart or not, which can't be avoided on SSR apps).

This alone makes me hesitant to upgrade, doing additional work for every reload that I only decide to make use of every few days is a big issue!

A --no-legacy flag with some form of console message as suggested by @spencern sounds like an appropriate start? That way it's purely opt-in.

@benjamn
Copy link
Member

@benjamn benjamn commented Jun 6, 2018

I like the idea of a --no-legacy command-line flag, since at least you have to keep passing it each time you run meteor, so there's less risk of accidentally disabling the legacy build permanently.

I'm still hopeful we can either generalize the command-line option to work with other build targets, not just web.browser.legacy; or find a way to remove the web.browser.legacy build from the critical rebuild pipeline altogether, without any explicit configuration. For example, just hypothetically, if client rebuilds did not block server restart, and the web.browser.legacy build happened asynchronously / after the web.browser build, then maybe the legacy build time would simply cease to matter?

@sebakerckhof
Copy link
Contributor

@sebakerckhof sebakerckhof commented Jun 6, 2018

I had a similar, but somewhat more generic idea some time ago: #9559 (comment)

@nethermead
Copy link

@nethermead nethermead commented Jun 7, 2018

If the increased build time for modern + legacy is the core issue in this thread, why not (also?) have the option (via command-line flag or what have you) to revert to the legacy-only build? In development, you often need a mix of speed along with widespread support (so looming clients/managers can see feature X on their crusty old browser Y). As a developer often saddled with such clients, the possibility that what they see with a legacy browser is broken compared to what I see in modern is just another opportunity for Murphy's law to kick me in my back end services.

@dobesv
Copy link

@dobesv dobesv commented Jun 8, 2018

For us the "modern" build breaks the app, so being able to go back to the 1.6 behavior would be great - just one legacy browser target. I wish I had time to come up with reproductions for all the bugs that popped up in 1.7 but they are numerous and really hard to track down nevermind extracting a useful reproduction.

@acomito
Copy link

@acomito acomito commented Jun 16, 2018

This was the first thing I noticed after upgrading. Even on a small project, it was much slower.

I like the no legacy flag idea. You just pop it in your npm script you use when running locally and you're good to go.

@veered
Copy link
Contributor Author

@veered veered commented Jun 20, 2018

@benjamn Any luck on this issue? We can't upgrade to 1.7 until there's some sort of workaround

@povesteam
Copy link
Contributor

@povesteam povesteam commented Jun 25, 2018

Please notify when there is a way to prevent waiting for the legacy build.

@anjunatic
Copy link

@anjunatic anjunatic commented Jun 26, 2018

--build-platforms web.browser cordova
... could be elegant solution

@veered
Copy link
Contributor Author

@veered veered commented Jul 4, 2018

I made a package which can let you choose which bundle you'd like to build: https://github.com/qualialabs/one

Unfortunately you must install it locally in your packages/ folder; you can't simply add it as an Atmosphere package.

Obviously it would be better to have a real solution, but for development this hack does the job. I have barely tested this at all... so user beware.

@benjamn
Copy link
Member

@benjamn benjamn commented Jul 4, 2018

Real solution coming very soon! In short: build all bundles when the build tool first starts, but then (for each subsequent rebuild) delay starting the web.browser.legacy build until after the other builds have finished and the development server has restarted. Once the web.browser.legacy build finishes, send an inter-process message to the webapp package running in the server process to tell it to reload the legacy bundle (since it loaded the stale bundle already). Any errors building the legacy bundle are still reported in the same way as before. Since the legacy bundle gets built automatically after the server restarts, you probably won't even notice the delay! 🇺🇸🆓🎆🎉

@benjamn
Copy link
Member

@benjamn benjamn commented Jul 5, 2018

Here it is! #10055

@mohammadidinani
Copy link

@mohammadidinani mohammadidinani commented Aug 6, 2018

I am trying to Migrate to Meteor 1.7.0.3 and I am facing long load time. Any idea why this is happening?

| Server startup...............................................73,588 ms (1)
| ├─ Load server bundles.......................................64,722 ms (1)
| │  ├─ packages/http.js..........................................149 ms (1)
| │  │  └─ require("/node_modules/meteor/http/httpcall_server.js").149 ms (1)
| │  │     └─ require("request")                                  148 ms (1)
| │  ├─ packages/webapp.js........................................125 ms (1)
| │  │  └─ require("/node_modules/meteor/webapp/webapp_server.js") 125 ms (1)
| │  ├─ packages/npm-mongo.js.....................................113 ms (1)
| │  │  └─ Npm.require("mongodb")                                 113 ms (1)
| │  ├─ packages/accounts-base.js..............................60,145 ms (1)
| │  │  └─ require("/node_modules/meteor/accounts-base/server_main.js") 60,145 ms (1)
| │  ├─ app/app.js..............................................3,452 ms (1)
@fangjj
Copy link

@fangjj fangjj commented Aug 29, 2018

nice! meteor update --release 1.7.1-beta.19

@stale
Copy link

@stale stale bot commented Dec 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Dec 10, 2019
@sebakerckhof
Copy link
Contributor

@sebakerckhof sebakerckhof commented Dec 11, 2019

still relevant

@stale stale bot removed the stale-bot label Dec 11, 2019
@filipenevola
Copy link
Member

@filipenevola filipenevola commented Dec 23, 2019

In progress here #10824

@SimonSimCity
Copy link
Contributor

@SimonSimCity SimonSimCity commented Jan 22, 2020

@filipenevola I think this can be closed, now that #10824 is merged - or is it better to keep it open until Meteor 1.10 (#10861) is merged and released?

@filipenevola filipenevola added this to the Release 1.10 milestone Jan 23, 2020
@filipenevola
Copy link
Member

@filipenevola filipenevola commented Jan 23, 2020

You can test this today using 1.10 beta. The current beta is 4 meteor update --release 1.10-beta.4 (more info here #10861)

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

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.