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

Fix filter parameter shadowing on locateIn #5062

Closed
wants to merge 1 commit into from

Conversation

gzzo
Copy link
Contributor

@gzzo gzzo commented Jul 2, 2019

The signature of locateIn includes a filter that is not used in the current definition of the function. This is causing some issues during the bundling on master for me. This is what locateIn and `locate ends up looking like:

    var locateVisible = function (container, current, selector) {
      return locateIn(container, current, selector);
    };
    var locateIn = function (container, current, selector, filter) {
      var predicate = curry(eq, current);
      var candidates = descendants(container, selector);
      var visible = filter(candidates, isVisible);
      return locate$2(visible, predicate);
    };

Note that the filter argument in locateVisible is removed, probably because the bundler can tell that the argument goes unused. Then locateIn calls filter from its argument list, which shadows the original Arr.filter, causing a filter is not a function error.

This is what the same block looks like from this branch:

    var locateVisible = function (container, current, selector) {
      var filter = isVisible;
      return locateIn(container, current, selector, filter);
    };
    var locateIn = function (container, current, selector, filter$1) {
      var predicate = curry(eq, current);
      var candidates = descendants(container, selector);
      var visible = filter(candidates, filter$1);
      return locate$2(visible, predicate);
    };

@TheSpyder
Copy link
Member

Thank you for highlighting this issue and finding a workaround.

This was caused by upgrading rollup last week before rollup/rollup#2970 was fixed. It has wider implications, we'll be upgrading rollup again to include the fix and doing a new release of TinyMCE shortly.

@TheSpyder TheSpyder closed this Jul 4, 2019
@gzzo
Copy link
Contributor Author

gzzo commented Jul 8, 2019

@TheSpyder Why not merge it? locateIn still has an unused argument in its signature that isn't used, regardless of rollup. Surprised lint doesn't catch this.

@TheSpyder
Copy link
Member

Our lint settings don't check for unused arguments at the moment. I didn't merge this because I was focussed on the critical issue in the release, which upgrading rollup fixed, not whether your change had other benefits.

Looking at the larger picture locateIn isn't even used directly so I might just inline it. Thanks for the reminder :)

lnewson pushed a commit that referenced this pull request Jul 11, 2019
* commit '1a5b57aa981994cd710f3a18570e6584643336d3':
  Inline the unused export locateIn, as highlighted in #5062
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