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

🏗 Update how to execute jscodeshift #32996

Merged
merged 5 commits into from Mar 3, 2021
Merged

Conversation

alanorozco
Copy link
Member

I'm not sure if something has changed, but transforms I've run locally have failed lately due to configuration issues. Copied default options, plus importAssertions.

I'm not sure if something has changed, but transforms I've run locally have failed lately due to configuration issues. Copied [default options](https://github.com/facebook/jscodeshift/blob/48f5d6d6e5e769639b958f1a955c83c68157a5fa/parser/babylon.js), plus `importAssertions`.
@alanorozco alanorozco requested a review from rsimha March 1, 2021 23:05
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM with a question. (Feel free to punt if this is blocking development.)

Instead of explicitly copy-pasting the default configs here and having to maintain this file when things change, is there a way to say this config file extends whatever is default?

@alanorozco
Copy link
Member Author

Instead of explicitly copy-pasting the default configs here and having to maintain this file when things change, is there a way to say this config file extends whatever is default?

No, I looked but couldn't find that. It doesn't seem like jscodeshift's config resolution is as sophisticated as Babel's.

@rsimha
Copy link
Contributor

rsimha commented Mar 1, 2021

No, I looked but couldn't find that. It doesn't seem like jscodeshift's config resolution is as sophisticated as Babel's.

I see. This might've changed in a recent release: https://github.com/facebook/jscodeshift/releases

The reason we didn't detect it as a breaking change is because we haven't version locked jscodeshift, and are instead, invoking it via npx:

const jscodeshift = (transform, args = []) =>
getStdoutThrowOnError(
[
'npx jscodeshift',
'--parser babylon',
`--parser-config ${__dirname}/jscodeshift/parser-config.json`,
`--transform ${__dirname}/jscodeshift/${transform}`,
...args,
].join(' ')
);

I'm not opposed to your adding it as a dependency and switching to the API instead of the CLI: https://www.npmjs.com/package/jscodeshift#the-jscodeshift-api. With this, we'll get a renovate PR for every new version, and we can preemptively detect and fix breaking changes.

@alanorozco
Copy link
Member Author

The jscodeshift api refers to the object passed in a transform's implementation, not the Runner itself. The Runner doesn't seem to be intentionally accessible, but some do use it. However, it didn't work for me.

This shouldn't be an issue for versioning though. npx jscodeshift will use the local copy if added to package.json.

  1. Added dependency to package.json.
  2. Moved to build-system/jscodeshift/parser-config.json so it can be shared. (mainly with 🏗 Collect z-index from JS files #32847)

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM after moving the config dir into test-configs/. (See comment below.)

This shouldn't be an issue for versioning though. npx jscodeshift will use the local copy if added to package.json

Excellent, this works too.

build-system/jscodeshift/README.md Outdated Show resolved Hide resolved
@alanorozco alanorozco changed the title 🏗 Update parser-config.json (sweep-experiments) 🏗 Update how to execute jscodeshift Mar 2, 2021
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.

None yet

3 participants