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

Updated to babel 7 and added React Fragment support #535

Merged
merged 6 commits into from
Jan 6, 2019

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Jan 1, 2019

Ran into issue #458 and worked out a fix for it and updated to babel 7 which required updating to the latest version of ava. I wasn't sure which branch you want this to be requested against since I saw mention of wanting to do the babel 7 update on v4 so let me know if I need to change it. Awesome project by the way!

and added support for React Fragments (fixes vercel#458)
@ijjk ijjk requested a review from giuseppeg as a code owner January 1, 2019 05:25
@giuseppeg
Copy link
Collaborator

@ijjk great work. This is an amazing PR, thank you!

I would like to publish this change as a minor release (rather than a breaking change) and keep supporting Babel 6. In order to do so we should keep the old tests in a subfolder say test/babel6 and run them using Babel 6, move the Babel configuration to a file (right now it is in package.json) and change ava's require to point to a local module (say test/_require) that conditionally returns babel-register or @babel/register.

Finally before releasing we should probably fix this #486 - we can do it in a follow up PR.

Let me know if you have time to do this. thank you again!

babel6, and pulled babel configs out of package.json
@ijjk
Copy link
Member Author

ijjk commented Jan 1, 2019

I updated my branch to prevent a breaking change and added the babel6 tests so it should work with both v6 and v7 now just that fragments won't work with v6 as it doesn't support the JSXFragment visitor from what I found.

I didn't find that ava's require needed to be mapped to both @babel/register and babel-register conditionally to get the tests to run with babel v6 and just edited the _transform module to use babel-core for the babel6 tests. Let me know if this isn't what you were wanting.

I can try to tackle #486 after this PR so we can get this fix out.

@giuseppeg
Copy link
Collaborator

@ijjk thank you! If you don't mind I'll wait a few days and test this branch on two real world apps to see if everything is fine.

@@ -90,7 +90,7 @@ export const getScope = path =>
).scope

export const isGlobalEl = el =>
el.attributes.some(({ name }) => name && name.name === GLOBAL_ATTRIBUTE)
el && el.attributes.some(({ name }) => name && name.name === GLOBAL_ATTRIBUTE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this check needed? I am talking about el &&

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openingElement is undefined for fragment paths from what I'm seeing so I added a check to make sure the el is defined to prevent an error.

@ijjk
Copy link
Member Author

ijjk commented Jan 2, 2019

Ok sounds good, thanks for taking a look at this!

src/babel.js Outdated
@@ -61,6 +61,15 @@ export default function({ types: t }) {
},
JSXElement: {
enter(path, state) {
// since JSXFragments don't visit JSXOpeningElement
// increment ignoreClosing for them here
if (t.isJSXFragment && t.isJSXFragment(path)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't work here when on the same file you have a fragment. Can you add this test to fixtures/fragment.js?

function Component1() {
  return (
     <>
         <div>test</div>
     </>
  )
}

function Component2() {
  return (
    <div>
      <style jsx>{`div { color: red }`}</style>
    </div>
  ) 
}

The good news is that we can register a JSXOpeningFragment:

  // only apply JSXFragment visitor if supported
  if (t.isJSXFragment) {
    visitors.visitor.JSXFragment = visitors.visitor.JSXElement
    visitors.visitor.JSXOpeningFragment = {
      enter(path, state) {
        if (!state.hasJSXStyle) {
          return
        }

        if (state.ignoreClosing === null) {
          // We keep a counter of elements inside so that we
          // can keep track of when we exit the parent to reset state
          // note: if we wished to add an option to turn off
          // selectors to reach parent elements, it would suffice to
          // set this to `1` and do an early return instead
          state.ignoreClosing = 0
        }

        state.ignoreClosing++
      }
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I updated the test for that case too.

Copy link
Collaborator

@giuseppeg giuseppeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ijjk thanks for the awesome work! I have one last request can you add a test to babel6 with a simple component that uses the fragment shorthand and assert that it throws? (just to make sure that it is running the tests with babel6).

Also can I get your twitter handle? I would like to give you credit for this :)

@ijjk
Copy link
Member Author

ijjk commented Jan 6, 2019

Yup, I added the babel 6 fragment shorthand test. Thanks for the awesome project!

I'm still working on setting up a twitter so it's still empty, but it's @_ijjk

@giuseppeg giuseppeg merged commit 58da7fb into vercel:master Jan 6, 2019
@giuseppeg
Copy link
Collaborator

awesome, merged thanks again <3

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.

2 participants