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

Array.map return types not removed #9129

Closed
anthonymuau opened this Issue Dec 5, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@anthonymuau

anthonymuau commented Dec 5, 2018

Bug Report

Current Behavior
As of flow 0.87.0 it forces you to add return types for array.map but return type is not always removed causing the javascript to break in the browser.

Input Code

  • REPL or Repo link if applicable:
_getChildrenElements = (items: ItemsType, size?: $Values<typeof Sizes>) => {
    const itemsLength = items.length - 1;
    return items.map<React$Node>((item: ItemType, index: number) => {
        const key = uuid();
        const extraProps = index < itemsLength ? {} : { href: undefined, onClick: undefined };
        const itemProps = {
          ...item,
          ...extraProps,
          key,
          size,
        };

        return <Item {...itemProps} />;
      });
  };

Expected behavior/code
A clear and concise description of what you expected to happen (or code).
I would Expect <React$Node> would be removed

Babel Configuration (.babelrc, package.json, cli command)

module.exports = {
presets: [
'@babel/preset-flow',
'@babel/preset-react',
[
'@babel/preset-env',
{
targets: {
browsers: ['last 2 versions', 'ie >= 11'],
},
useBuiltIns: 'entry',
},
],
],
plugins: [
'@babel/plugin-transform-flow-strip-types',
'@babel/plugin-proposal-class-properties',
'@babel/plugin-syntax-dynamic-import',
],
};

"@babel/core": "^7.2.0",
"@babel/plugin-proposal-class-properties": "^7.0.0",
"@babel/plugin-proposal-json-strings": "^7.0.0",
"@babel/plugin-syntax-dynamic-import": "^7.0.0",
"@babel/plugin-transform-flow-strip-types": "^7.2.0",
"@babel/polyfill": "^7.0.0",
"@babel/preset-env": "^7.2.0",
"@babel/preset-flow": "^7.2.0",
"@babel/preset-react": "^7.0.0",
"@babel/register": "^7.2.0",
"@babel/runtime": "^7.2.0",
 _getChildrenElements = (items: ItemsType, size?: $Values<typeof Sizes>) => {
    const itemsLength = items.length - 1;
    return items.map((item: ItemType, index: number) => {
        const key = uuid();
        const extraProps = index < itemsLength ? {} : { href: undefined, onClick: undefined };
        const itemProps = {
          ...item,
          ...extraProps,
          key,
          size,
        };

        return <Item {...itemProps} />;
      });
  };

Environment

  • Babel version(s): [e.g. v6.0.0, v7.0.0-beta.34]
  • Node/npm version: [e.g. Node 8/npm 5]
  • OS: [e.g. OSX 10.13.4, Windows 10]
  • Monorepo [e.g. yes/no/Lerna]
  • How you are using Babel: [e.g. cli, register, loader]

Possible Solution

Additional context/Screenshots
Add any other context about the problem here. If applicable, add screenshots to help explain.

@babel-bot

This comment has been minimized.

Collaborator

babel-bot commented Dec 5, 2018

Hey @anthonymuau! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@nicolo-ribaudo

This comment has been minimized.

Member

nicolo-ribaudo commented Dec 5, 2018

Have you added // @flow at the beginning of the file? Some features can only be enabled like that https://babeljs.io/docs/en/babel-parser#plugins-options.

@anthonymuau

This comment has been minimized.

anthonymuau commented Dec 5, 2018

@nicolo-ribaudo I do have flow comment at the top and babel does remove all flow types but <React$Node>

/* eslint-disable prettier/prettier */
/* eslint-disable css-modules/no-unused-class */
// @flow

import * as React from 'react';

import classnames from 'classnames';
import debounce from 'lodash.debounce';
import uuid from 'uuid/v4';

import { Item } from './Internal/Item';
import { MoreItems } from './Internal/MoreItems';
import {
  addEventListener,
  getRealDomNode,
  setStyleOnElement,
  removeStyleOnElement,
  visibleElement,
} from '../../ace-internal/util/dom';

import styles from './styles.scss';

import type { testIdPropType, ansaradaCCDPropType } from '../../ace-internal/types/general';

const Sizes = Object.freeze({
  Small: 'Small',
  Medium: 'Medium',
});

export type BreadcrumbArrayType = Array<React.Element<typeof Item>>;

type ItemBaseType = {|
  text: string,
  ...testIdPropType,
  ...ansaradaCCDPropType,
|};

type ItemUrlType = {|
  href: string,
  target?: string,
  ...ItemBaseType,
|};

type ItemOnClickType = {|
  onClick: () => void,
  ...ItemBaseType,
|};

export type ItemType = ItemUrlType | ItemOnClickType;
export type ItemsType = Array<ItemType>;

export type BreadcrumbsPropType = {|
  items: ItemsType,
  className?: string,
  size: $Values<typeof Sizes>,
  ...testIdPropType,
|};

type BreadcrumbsState = {
  collapsedChildrenArray: ItemsType,
};

class Breadcrumbs extends React.Component<$Exact<BreadcrumbsPropType>, BreadcrumbsState> {
  _wrapperNode: ?HTMLElement;

  _moreNode: ?HTMLElement;

  _unbindHandlers: Array<() => void> | void;

  _resourcesLoadedEvent: Function | void;

  _windowResizedTimer: TimeoutID;

  _debouncedWindowResize: Function;

  _moreButtonSize: number;

  static defaultProps = {
    size: Sizes.Medium,
  };

  constructor() {
    super();
    this._moreButtonSize = 56;
    this._debouncedWindowResize = debounce(this._requestUpdateDisplayedItems, 100);
    this.state = { collapsedChildrenArray: [] };
  }

  _handleEvents = () => {
    if (this._unbindHandlers === undefined || this._unbindHandlers.length === 0) {
      this._unbindHandlers = [addEventListener(window, 'resize', this._windowResize)];
    }
  };

  _unBindEvents = () => {
    if (this._unbindHandlers !== undefined) {
      this._unbindHandlers.forEach(func => func());
      this._unbindHandlers = undefined;
    }
  };

  _resetDisplayedItems = (wrapperElement: HTMLElement) => {
    Array.from(wrapperElement.querySelectorAll('li'))
      .filter(item => item !== this._moreNode)
      .forEach((item: HTMLElement) => {
        removeStyleOnElement(item, 'display');
        item.classList.remove(styles.isLastLong);
        item.classList.remove(styles.isSecondLastLong);
      });
  };

  _calcOverflowWidth = (DomElementsListWidth: Array<number>, wrapperWidth: number) =>
    DomElementsListWidth.reduce((accumulator, currentValue) => accumulator + currentValue, 0) -
    wrapperWidth;

  _windowResize = () => {
    this._debouncedWindowResize();
  };

  _updateDisplayedItemsNow = () => window.requestAnimationFrame(this._requestUpdateDisplayedItems);

  _requestUpdateDisplayedItems = () =>
    this._wrapperNode ? this._renderDisplayedItems(this._wrapperNode, this.props.items) : undefined;

  _renderDisplayedItems = (wrapperElement: HTMLElement, items: ItemsType) => {
    visibleElement(wrapperElement, false);
    this._resetDisplayedItems(wrapperElement);
    const DomElementsList = Array.from(wrapperElement.querySelectorAll('li')).filter(
      item => item !== this._moreNode,
    );
    const DomElementsListWidth = DomElementsList.map(item => item.clientWidth);
    const overFlowWidth =
      this._calcOverflowWidth(DomElementsListWidth, wrapperElement.clientWidth) - 1;
    const collapsedChildrenArray = [];

    if (overFlowWidth > 0) {
      let domElementsRemoveWidth = 0;

      DomElementsList.forEach((domElement, index) => {
        if (
          domElementsRemoveWidth <= overFlowWidth + this._moreButtonSize &&
          collapsedChildrenArray.length < items.length - 2
        ) {
          collapsedChildrenArray.push(items[index]);
          domElementsRemoveWidth += DomElementsListWidth[index];
          setStyleOnElement(domElement, 'display', 'none');
        }
      });
    }

    if (items.length > 1 && collapsedChildrenArray.length === items.length - 2) {
      DomElementsList[DomElementsList.length - 1].classList.add(styles.isLastLong);
      DomElementsList[DomElementsList.length - 2].classList.add(styles.isSecondLastLong);
    }

    if (this.state.collapsedChildrenArray.length !== collapsedChildrenArray.length) {
      this.setState({ collapsedChildrenArray });
    }

    visibleElement(wrapperElement, true);
  };

  _resourcesHasLoaded = () => {
    this._updateDisplayedItemsNow();
    this._resourcesLoadedEvent = undefined;
  };

  _handleRunOnceEvents = () => {
    if (document.readyState === 'loading') {
      this._resourcesLoadedEvent = window.addEventListener('load', this._resourcesHasLoaded);
    } else {
      this._updateDisplayedItemsNow();
    }
  };

  _getChildrenElements = (items: ItemsType, size?: $Values<typeof Sizes>) => {
    const itemsLength = items.length - 1;
    return items.map<React$Node>((item: ItemType, index: number) => {
        const key = uuid();
        const extraProps = index < itemsLength ? {} : { href: undefined, onClick: undefined };
        const itemProps = {
          ...item,
          ...extraProps,
          key,
          size,
        };

        return <Item {...itemProps} />;
      });
  };

  componentDidMount() {
    this._handleEvents();
    this._handleRunOnceEvents();
  }

  componentDidUpdate() {
    this._updateDisplayedItemsNow();
  }

  componentWillUnmount() {
    this._unBindEvents();
    this._debouncedWindowResize.cancel();
  }

  render() {
    const { items, size, 'data-test-id': testId, className } = this.props;

    return (
      <div
        ref={el => {
          this._wrapperNode = el;
        }}
        style={{ visibility: 'hidden', maxWidth: '100%', minWidth: '100%', overflow: 'hidden' }}
        data-test-id={testId}
      >
        <ul className={classnames(styles.breadcrumbs, className)}>
          {this.state.collapsedChildrenArray.length > 0 && (
            <MoreItems
              items={this.state.collapsedChildrenArray}
              ref={el => {
                this._moreNode = getRealDomNode(el);
              }}
            />
          )}
          {this._getChildrenElements(items, size)}
        </ul>
      </div>
    );
  }
}

export { Breadcrumbs, Sizes };
@anthonymuau

This comment has been minimized.

anthonymuau commented Dec 6, 2018

Here is a repo with cut down version of the project
https://bitbucket.org/anthonymuau/babel-bug-test/src/master/

@nicolo-ribaudo

This comment has been minimized.

Member

nicolo-ribaudo commented Dec 6, 2018

I will look at it later today

@anthonymuau

This comment has been minimized.

anthonymuau commented Dec 9, 2018

@nicolo-ribaudo thanks take your time :)

@existentialism

This comment has been minimized.

Member

existentialism commented Dec 9, 2018

Using your repo, if I run,

npx babel source/index.js

The checked in file in the repo does not have the flow pragma, so it produces:

var _getChildrenElements = function _getChildrenElements(items) {
  return items.map < React$Node > function (item) {
    return item.text;
  };
};

With // @flow atop file, it correctly removes it:

var _getChildrenElements = function _getChildrenElements(items) {
  return items.map(function (item) {
    return item.text;
  });
};

Curious, what version of babel-related deps are in your lockfile in your project?

@anthonymuau

This comment has been minimized.

anthonymuau commented Dec 10, 2018

@existentialism i just worked out based on what you said. it's because on the real version of this file we had comment on top of // @flow for eslint. I move the order so //@flow was first and worked. what was sooo odd with it is all the other flow types where removed just not that one.

@existentialism

This comment has been minimized.

Member

existentialism commented Dec 10, 2018

@anthonymuau to be clear, the items.map<React$Node>() syntax was added via #7934, the following is important:

Since this is a Flow-only feature, the parsing rules are conditional. We will only attempt to parse type arguments if either (a) the all option, added in 1c359ad is provided to babel-plugin-syntax-flow or (b) the file starts with a docblock comment containing the @flow or @noflow pragma.

Note, this matches flow's behavior:

Flow will only attempt to parse this syntax as explicit type arguments if
should_parse_types is true -- i.e., if Flow is enabled for the file, either
through the flow pragma or the --all cmdline argument.

facebook/flow@40fbcdd

This issue is probably sneaking up on people, since as you mentioned, flow@0.87 forces you to add return types to Array.map!

@anthonymuau

This comment has been minimized.

anthonymuau commented Dec 10, 2018

@existentialism thanks!

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