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

[core] Switch to Typescript incrementally (tests run @babel/register) #9561

Closed
wants to merge 95 commits into from

Conversation

rosskevin
Copy link
Member

@rosskevin rosskevin commented Dec 20, 2017

Variation on #9535. This uses no typescript runtime of any kind -it is all babel - just as we use on the current v1-beta (except babel 7.x).

What must be fixed

  • build:es - failing again - not ignoring *.d.ts (cannot have these in .babelrc) and cli ignore wasn't catching them
  • yarn test:unit - 11 broken tests - many related to ref and findDOMNode - perhaps due to babel 7 update
  • next.js build issue (details in 9535)

What can wait but should be fixed eventually

  • one omitted babel plugin transform-dev-warning - not updated to babel 7
  • Unable to process function body node type: ArrowFunctionExpression - possibly istanbul/babel 7 - not sure if this could cause other problems

Completed

  • js/ts/tsx all work in mocha
  • converted internal utils/styles etc to ts
  • converted a few components to ts for testing
  • converted a few specs to ts for testing
  • fixed known build issues - yarn build runs - updated to babel 7.

Effectively, we use tsc like we did flow, and tsc is uninvolved in the build/runtime process.

@rosskevin
Copy link
Member Author

@agamrafaeli you wrote the tests in Menu that are now failing with the babel 7 upgrade. I admit I don't know much about how it was intended to work, but it is failing now. Would you mind taking a look? Please note that this is not related to typescript at all - this is 100% babel (just babel 7 instead of 6)

Here are the specifics:

In menu.spec - instance.menuList = {} appears to be problematic as ReactDOM.findDOMNode(this.menuList) is invalid as I see it - this is a fake object and would not have a dom node in this spec code:

    describe('menuList.selectedItem exists', () => {
      before(() => {
        instance.menuList = {};
        instance.menuList.selectedItem = SELECTED_ITEM_KEY;
      });

yielding this error which seems appropriate AFAIK (also see circleci logs):

Invariant Violation: Element appears to be neither ReactComponent nor DOMNode. Keys: selectedItem
      at invariant (node_modules/fbjs/lib/invariant.js:42:15)
      at Object.findDOMNode (node_modules/react-dom/cjs/react-dom.development.js:15282:7)
      at Menu.value [as handleEnter] (src/Menu/Menu.js:72:31)

Any help is appreciated @agamrafaeli

@kgregory
Copy link
Member

@rosskevin stubbing ReactDOM.findDOMNode doesn't seem to be working, but you can try stuffing a nodeType into menuList so that findDOMNode hands you the object back:

    it('should call menuList focus when menuList but no menuList.selectedItem ', () => {
      const ELEMENT_NODE = 1;
      // convince react-dom to pass the node back
      instance.menuList = { ...menuListSpy, nodeType: ELEMENT_NODE };
      delete instance.menuList.selectedItem;
      instance.handleEnter(elementForHandleEnter);
      assert.strictEqual(selectedItemFocusSpy.callCount, 0);
      assert.strictEqual(menuListFocusSpy.callCount, 1);
    });

You should be able to do this in the rest of the tests like this.

@rosskevin
Copy link
Member Author

@kgregory this worked for several, but not all. I figured out that findDOMNode is supposed to be stubbed and it is actually not being called - so that is the source of the difference.

@rosskevin
Copy link
Member Author

rosskevin commented Dec 21, 2017

Filed babel/babel#7088 related to the issues we are seeing with stubbing in Menu.spec and Menu with findDOMNode.

This can be fixed other ways if someone is interested in contributing.

@mbrookes mbrookes changed the title Switch to Typescript incrementally (tests run @babel/register) [core] Switch to Typescript incrementally (tests run @babel/register) Dec 22, 2017
@rosskevin
Copy link
Member Author

Theoretically @oliviertassinari typescript support has been added to next.js vercel/next.js#3578 (comment)

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 30, 2018

@rosskevin I'm happy they switched to using webpack for the server. It will solve a bunch of problem in my app with Next.js.

@oliviertassinari
Copy link
Member

@rosskevin I'm closing. I fear this pull-request is already quite behind HEAD. I don't think it was a waste of time. I believe we have now a better understanding of the implications of using TypeScript. So thanks! I think that we gonna have to start by upgrading 1. Babel and 2. Next.js.

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

Successfully merging this pull request may close these issues.

3 participants