-
Notifications
You must be signed in to change notification settings - Fork 835
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 unused exports and modules #219
Open
raphinesse
wants to merge
11
commits into
apache:master
Choose a base branch
from
raphinesse:purge
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
raphinesse
requested review from
purplecabbage,
janpio,
dpogue,
timbru31,
erisu,
brodycj and
breautek
November 3, 2019 23:11
erisu
approved these changes
Nov 4, 2019
can this be closed? is it still valid? |
I won't be able to see this through any time soon. If there is no value in leaving this PR open as a reminder to others, please feel free to close it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Platforms affected
All
Motivation and Context
I'm always trying to reduce the footprint of our code base by removing unused code. Thus this PR removes a bunch of exports that are not being used by any official
apache/cordova-*
repository.Unfortunately, while many of our modules that are available through
cordova.require
seem like they are intended for internal use only and there is no public documentation of them either (at least not to my knowledge), they are still being used in the wild by third party plugins (I checked it for one or two more unique names via GH search). Thus we will have to tread lightly here.I'm putting this PR up for awareness and discussion. It would be great if we could somehow get an idea of the impact of these changes. That would allow us to make an informed decision on whether we should deprecate all or some of the exports first. But unfortunately GH search seems to be too fuzzy to efficiently search for usages of these exports. I'm open for ideas.
Description
This change removes the following exports and modules that are available via
cordova.require
cordova/builder
cordova/modulemapper
. Had no tests of its own either. Thus I integrated the used functions intocordova/modulemapper
.cordova/utils
alert
: roughlymsg => (window.alert || console.log)(msg)
arrayIndexOf
: legacy substitute forArray.prototype.indexOf
arrayRemove
: Find item by identity and remove from Arrayclose
: legacy substitute forFunction.prototype.bind
isArray
: legacy substitute forArray.isArray
isDate
:d => d instanceof Date
cordova/argscheck
enableChecks
: module variable to disable all checks. Bad design IMHOcordova/modulemapper
defaults
: likeclobbers
but does nothing if property already existscordova.addConstructor
: basicallyfn => channel.onCordovaReady.subscribe(fn)
Testing
npm t