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

[evcohen/jsx-ast-utils#70] Support OptionalMemberExpression AST nodes #77

Merged

Conversation

jessebeach
Copy link
Collaborator

@jessebeach jessebeach commented Mar 18, 2019

Support patterns like

<div foo={b?.c?.d;} />

I made the ESLint changes necessary to upgrade from Babel 6 to Babel 7, although the kept the old dependencies. The upgrade should be easy and now it'll be easier to test locally with Babel 7 and not get a ton of ESLint warnings (because ESLint will upgrade as well and start complaining about cyclical imports).

ljharb
ljharb previously requested changes Mar 18, 2019
package.json Outdated
@@ -54,6 +57,7 @@
]
},
"dependencies": {
"@babel/polyfill": "^7.2.5",
Copy link
Member

Choose a reason for hiding this comment

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

I’d strongly prefer to avoid this; i don’t want core-is in my dev dep graph. Why is it needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. It was there before. Let me try removing it.

@jessebeach jessebeach force-pushed the fix-OptionalObjectExpression branch 2 times, most recently from 6e041ca to cf99ce4 Compare March 18, 2019 02:32
@@ -1,11 +1,9 @@
import getValue from './index';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to get rid of all these circular dependencies.

@coveralls
Copy link

coveralls commented Mar 18, 2019

Coverage Status

Coverage decreased (-1.01%) to 97.838% when pulling 3bff7c8 on jessebeach:fix-OptionalObjectExpression into a9d4159 on evcohen:master.

@jessebeach jessebeach requested a review from ljharb March 18, 2019 02:34
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Instead of forcing getValue to be injected everywhere, could you use a lazy require?

@ljharb ljharb dismissed their stale review March 18, 2019 03:45

core-js is gone, removing block

@jessebeach jessebeach force-pushed the fix-OptionalObjectExpression branch 2 times, most recently from 2222861 to 02384d5 Compare March 18, 2019 04:53
@jessebeach
Copy link
Collaborator Author

@ljharb how do you recommend we deal with the optional chaining plugin in Babel 7 which means we need to drop support for Node 4 and 5, since Babel 7 doesn't support them?

@jessebeach
Copy link
Collaborator Author

Instead of forcing getValue to be injected everywhere, could you use a lazy require?

@ljharb like this in a function block?

const getValue = require('./index.js').default;

I'm worried that this will incur an extra synchronous file read and slow down the process. Passing the function around isn't terrible. Plus I'd have to suppress just as many global require lint warnings as circular import warnings that I was trying to fix with this change:

8:20  error  Unexpected require()  global-require

@ljharb
Copy link
Member

ljharb commented Mar 18, 2019

It would only incur a file read once; after which there's a require cache (which import utilizes since babel just transpiles it to require). You'd certainly have to suppress global-require warnings, true.

As for optional chaining; perhaps we could move the project to babel 7, but make a special travis.yml script to uninstall babel 7 and install babel 6 (and a helper file to handle the conditional requires, and perhaps conditional skips) so we can keep testing in those versions?

@jessebeach
Copy link
Collaborator Author

As for optional chaining; perhaps we could move the project to babel 7, but make a special travis.yml script to uninstall babel 7 and install babel 6 (and a helper file to handle the conditional requires, and perhaps conditional skips) so we can keep testing in those versions?

@ljharb only the built lib files need to run in Node 4 and 5, right? I wouldn't expect the project to build in Node 4 or 5 as a requirement, since only developers working on this project need to build it. I added a node target to the babelrc to build for Node 4, so that the necessary transforms run.

"presets": [
    [
      "@babel/preset-env", {
        "targets": {
          "node": "4.0",
        }
      }
    ]
  ],

@ljharb
Copy link
Member

ljharb commented Mar 18, 2019

True, so that's another alternative - in travis, we'd have nvm install lts/*, run the build process, and then nvm use "${TRAVIS_NODE_VERSION}". Slightly slower but certainly cleaner.

@jessebeach
Copy link
Collaborator Author

@ljharb You definitely have more exposure to the wider Node world. Is Node 4/5 support still a hard line?

@jessebeach
Copy link
Collaborator Author

True, so that's another alternative - in travis, we'd have nvm install lts/*, run the build process, and then nvm use "${TRAVIS_NODE_VERSION}". Slightly slower but certainly cleaner.

@ljharb , We'd need to build the __tests__ directory in Node 6+, switch to Node 4/5, then run Jest with the transforms turned off and pegged to version 23 (but only in Node 4/5); we can use Jest 24+ with Node 6+.

This all sounds doable (although I won't be doing it tonight 😄). But my question is, is it completely necessary? Are other projects maintaining support for Node 4/5? And if yes, how many years will we need to keep up this support in your opinion?

@ljharb
Copy link
Member

ljharb commented Mar 18, 2019

I mean, node < 6 is EOL, and 6 will be EOL soon. But while eslint still supports a version of node, I think we should still support it.

Unfortunately jest and mocha both have dropped support for older nodes, so at this point, I recommend that jest be used for apps, and tape for everything else.

When eslint 6 comes out, I think we could drop support for eslint < 5, which would allow us to drop node < 4. However, I think if a tool is going to force dropping support, then the tool is broken, and the solution isn't to cut off those users.

All of my projects maintain support for node 0.6, for as many decades as that ends up being practical. Humans are more important than tools :-) (eslint and babel projects, ofc, tend to support node 4+, since both projects have dropped node < 4 and there's nothing i can do about that)

@jessebeach
Copy link
Collaborator Author

@ljharb I set the Node versions back to 4, 5, 6. I spent the whole day trying to get the project to run in a Babel 6 and Babel 7 world, but I was unsuccessful. So for now, I'm going to make "make the project work" in Babel 7, but the code will test Babel 6 (Node 4, 5 and 6). That's where we're stuck until we drop support for Node 4 and 5.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

It'd be nice to find a better away around the circular deps, but this LGTM.

@jessebeach
Copy link
Collaborator Author

It'd be nice to find a better away around the circular deps, but this LGTM.

Agree, I'm not in love with it either. But inspiration is not arriving to me with consistency lately :)

@jessebeach jessebeach merged commit b5debad into jsx-eslint:master Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants