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 underscore from meteor-base #9596

Merged
merged 4 commits into from Mar 7, 2018

Conversation

Projects
None yet
4 participants
@jamesmillerburgess
Contributor

jamesmillerburgess commented Jan 29, 2018

This should shave down bundle sizes by 14.4 kb for many non-blaze projects.

The other core meteor packages have not depended on underscore since #9362. However, we are only able to remove this last dependency now due to the the first commit in this PR, which eliminates usages of underscore from apps that did not have the package listed in their packages files. This was causing CI test failures that now should be corrected.

Any meteor apps currently using _ without underscore listed in their packages file will need to add the package explicitly.

meteor-base version number bumped from 1.3.0 to 1.3.1.

Adjustments to test apps:

There are only a few uses of underscore in these apps, and two of them actually used underscore without having it explicitly listed in their packages file.

This is a problem, because the apps were relying on the dependency from meteor-base, which we want to remove to cut down bundle sizes.

For the modules test app, I've added underscore to the packages file, because it is asserting that require('meteor/underscore')._ equals _. For the other app and all other uses of _, rather than add underscore to the packages files, I took the modernization route and replaced the functions with their ES6 equivalents, and then removed underscore from all packages files.

@benjamn

This comment has been minimized.

Member

benjamn commented Jan 30, 2018

@hwillson The passwords - tokens test failure reproduces consistently for me, and bisects to the Mongo 3.6 upgrade. I think the new version of Mongo fires oplog notifications with slightly different timing, and thus this comment becomes relevant. I think we need to fix this, because tests are going to keep failing for most PRs until it's fixed.

@jamesmillerburgess jamesmillerburgess changed the base branch from devel to release-1.6.2 Jan 30, 2018

@jamesmillerburgess jamesmillerburgess changed the base branch from release-1.6.2 to devel Jan 30, 2018

@hwillson

This comment has been minimized.

Member

hwillson commented Jan 30, 2018

Ouch, good catch @benjamn - I'll take a look. Thanks!

@jamesmillerburgess jamesmillerburgess force-pushed the jamesmillerburgess:remove-underscore-from-core branch from 340799a to b87a650 Jan 30, 2018

@jamesmillerburgess

This comment has been minimized.

Contributor

jamesmillerburgess commented Jan 30, 2018

I've fixed the other error on the original PR - the custom-minifier app was missing the ecmascript package. I've also added an entry to History.md, and I think the AppVeyor failure is due to me force-pushing these changes.

@abernix

This comment has been minimized.

Member

abernix commented Jan 30, 2018

First, thank you so much for taking the time to work on great improvements like this.

AppVeyor won't build if there are any merge conflicts with the target branch as they build against a virtual GitHub branch which is meant to represent the "merge result", in an effort to try to provide early warning of what might eventually be a build a broken build. Currently, that merge conflict is just History.md so if you rebase against devel you might have luck fixing that. Probably not necessary to worry about right now given that you're not making any Windows-specific changes.

Lastly, thanks again for taking the time to work on great improvements like this. 😉🙏👏

@jamesmillerburgess

This comment has been minimized.

Contributor

jamesmillerburgess commented Jan 30, 2018

@abernix Thanks for going out of your way to help contributors like me!

@hwillson

hwillson requested changes Jan 30, 2018 edited

This is an awesome change @jamesmillerburgess, thanks!

The code changes look great, so I don't have any feedback there.

I am concerned that this change will inevitably upset people who don't have underscore explicitly added to their project. What do people think - are we okay with just documenting this in the History.md and letting people figure things out when they get application errors because underscore is now missing?

I'll mark this a request changes even though the code is okay, just until we hash out a plan of attack for making this change as painless as possible for existing users.

@jamesmillerburgess

This comment has been minimized.

Contributor

jamesmillerburgess commented Jan 30, 2018

@hwillson

I agree with you that it isn't nice to introduce breaking changes without some kind of deprecation warning, although I can't think of an elegant way to do that for this particular situation:

  1. An app is using _ specifically for underscore functionality,
  2. the only meteor package it is using that depends on underscore is meteor-base, and
  3. they also are not importing an npm package like lodash, underscore, or something else as _ for the same purpose

Besides being difficult to detect for a deprecation warning, that's a pretty odd set of criteria, so I'm not even sure there are non-trivial apps out there that meet them. Hopefully that means the blast radius will be quite small if we go forward with the change.

@hwillson

This comment has been minimized.

Member

hwillson commented Jan 31, 2018

I agree @jamesmillerburgess, I couldn't think of a better way of handling this either. I guess I'm just envisioning complaints rolling in from people who are used to just running meteor create my-happy-app and assuming they can still use underscore without doing anything additional, because once upon a time this was the accepted way of doing things (by leveraging the _ global). Even though this approach is no longer recommended, people can still do it (and ignore progress), until we merge this PR.

That being said, I like progress, so my vote goes towards exactly what you've done - mention this in the History.md. We can also highlight this in a Meteor 1.6.3 release blog post, and on the forums / in the Guide if necessary. I just thought I'd mention this so we're all on the same page, and are ready with answers in-case (a hopefully very small group of) people are upset.

Okay, enough rambling on my part - approved! 🙂

@@ -69,7 +69,7 @@ C.find().observeChanges({
Meteor.clearTimeout(nextStepTimeout);
nextStepTimeout = null;
}
if (!fields.step && _.has(steps, fields.step)) {
if (!fields.step && steps.hasOwnProperty(fields.step)) {

This comment has been minimized.

@benjamn

benjamn Feb 1, 2018

Member

Generally we prefer Object.prototype.hasOwnProperty.call(steps, fields.step), but since this is a test and it passes, I think this is fine. The rationale is that hasOwnProperty could be a field in the steps object, which would mean steps.hasOwnProperty !== Object.prototype.hasOwnProperty.

This comment has been minimized.

@jamesmillerburgess

jamesmillerburgess Feb 1, 2018

Contributor

Ah, I was wondering why I was seeing it used like that elsewhere. I will update my other PRs as well. Thanks!

@@ -1,6 +1,6 @@
Package.describe({
name: 'meteor-base',
version: '1.3.0',
version: '1.3.1',

This comment has been minimized.

@benjamn

benjamn Feb 1, 2018

Member

Let's bump this to 1.4.0 so that the changes won't take effect until developers update to Meteor 1.6.2. This kind of change is appropriate for 1.6.2, I think, because 1.6.2 is all about various bundle size optimizations.

@jamesmillerburgess jamesmillerburgess force-pushed the jamesmillerburgess:remove-underscore-from-core branch from 576501d to a91b407 Feb 1, 2018

@benjamn

benjamn requested changes Feb 8, 2018 edited

After talking about this in the issue triage meeting, we think decoupling underscore from meteor-base is a good idea, but we're afraid that lots of apps will be broken when they update, so it's important to add underscore back, separately from meteor-base. The way to do this is to write an "upgrader" that modifies the .meteor/packages file by adding underscore if meteor-base is also used, assuming underscore is not already there. This upgrader will run during meteor update. See this file for lots of examples of other upgraders we've used over the years.

Thanks for pushing this forward!

@hwillson

This comment has been minimized.

Member

hwillson commented Feb 8, 2018

Just to add to #9596 (review), if you don't have time to look into writing the upgrader, let me know and I'll dig into it. Thanks!

@jamesmillerburgess

This comment has been minimized.

Contributor

jamesmillerburgess commented Feb 8, 2018

@jamesmillerburgess jamesmillerburgess force-pushed the jamesmillerburgess:remove-underscore-from-core branch from 4c8738f to da2477a Feb 8, 2018

jamesmillerburgess added some commits Jan 29, 2018

Fix underscore dependencies in test apps
There are only a few uses of `underscore` in these apps, and two of them actually used `underscore` without having it explicitly listed in their `packages` file.

This is a problem, because the apps were relying on the dependency from `meteor-base`, which we want to remove to cut down bundle sizes.

For the `modules` test app, I've added `underscore` to the `packages` file, because it is using `_` in an assertion about the module system. For the other app and all other uses of `_`, rather than add `underscore` to the `packages` files, I took the modernization route and replaced the functions with their ES6 equivalents, and then removed `underscore` from all `packages` files.
Remove underscore as a dependency of meteor-base
This should shave down bundle sizes by 14.4 kb for many non-blaze projects.

The other core meteor packages have not depended on `underscore` since #9362. However, we are only able to remove this last dependency now due to the previous commit, which eliminated usages of `underscore` from apps that did not have the package listed in their `packages` files. This was causing CI test failures that now should be corrected.

Any meteor apps currently using `_` without `underscore` listed in their `packages` file will need to add the package explicitly.

Version number bumped from 1.3.0 to 1.3.1.

@jamesmillerburgess jamesmillerburgess force-pushed the jamesmillerburgess:remove-underscore-from-core branch from f0f8ca7 to 10620a4 Feb 9, 2018

@jamesmillerburgess

This comment has been minimized.

Contributor

jamesmillerburgess commented Feb 9, 2018

I tested the upgrader with a simple app and it seemed to do what it was supposed to for each of the conditions. I also adjusted the entry in History.md to reflect the presence of the upgrader and the implications for developers.

@benjamn

benjamn approved these changes Mar 7, 2018

Looking good! We'll get this into a beta soon!

@benjamn benjamn merged commit 4ebf089 into meteor:devel Mar 7, 2018

17 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Group 11 Your tests passed on CircleCI!
Details
ci/circleci: Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjamn benjamn added this to the Release 1.6.2 milestone Mar 7, 2018

@skirunman skirunman referenced this pull request Mar 8, 2018

Closed

Modernize package #24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment