fix(commonjs): correctly replace shorthand require
#764
Conversation
shellscape
requested review from
LarsDenBakker,
lukastaegert,
tjenkinson and
TrySound
chmelevskij
force-pushed
the
fix/commonjs-require-shorthand
branch
from
0afb9fe
to
4b83f0b
Compare
packages/commonjs/src/ast-utils.js
Outdated
| @@ -115,3 +115,7 @@ export function isLocallyShadowed(name, scope) { | |||
| } | |||
| return false; | |||
| } | |||
|
|
|||
| export function isShorthandProperty(parent) { | |||
| return parent && parent.shorthand; | |||
There was a problem hiding this comment.
Please can you check parent.type is the correct value too?
There was a problem hiding this comment.
Looks good, but agree with @tjenkinson 's comment. I might not cause issues now, but who knows about the future.
There was a problem hiding this comment.
Actually, searching through the entire https://github.com/estree/estree/search?q=shorthand there is only one potential upcoming use-case for shorthand, which would be RecordProperty, and it seems to me that those would benefit from the same treatment as regular Property. So from my side, we do not necessarily need this right now.
IMO this still applies and not checking the type feels similar to leaving a dependency version as I'm assuming a new type in the parser wouldn't need to be major |
|
@lukastaegert @tjenkinson is the consensus that this isn't needed at the moment, or that it's useful and we should merge but keep an eye out for potential issues in the future? |
|
With the last changes, I think consensus is to merge this. |
|
|
Rollup Plugin Name:
commonjsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: #763
Description
Checks that
requireis shorthand property.