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

#4964: Permanently move JQ out of sandbox #6579

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Oct 1, 2023

What does this PR do?

JQ hasn't been run in the sandbox since #4972. On Slack, Todd and I figured that maybe JQ doesn't actually need to run in the sandbox even in MV3, so I'm moving it out permanently.

I think this comment was the reason why it was added to the sandbox in the first place:

Related

Checklist

@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (b4d2d22) 70.05% compared to head (ab6d50e) 70.06%.
Report is 26 commits behind head on main.

❗ Current head ab6d50e differs from pull request most recent head baa0dab. Consider uploading reports for the commit baa0dab to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6579   +/-   ##
=======================================
  Coverage   70.05%   70.06%           
=======================================
  Files        1175     1176    +1     
  Lines       36624    36621    -3     
  Branches     6913     6913           
=======================================
+ Hits        25656    25657    +1     
+ Misses      10968    10964    -4     
Files Coverage Δ
src/bricks/transformers/jq.ts 89.13% <100.00%> (ø)
src/sandbox/messenger/api.ts 0.00% <ø> (ø)
src/sandbox/messenger/executor.ts 94.44% <ø> (-1.02%) ⬇️
src/sandbox/messenger/registration.ts 0.00% <ø> (ø)
src/utils/jq.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/utils/jq.ts Outdated
filter: string;
};

export async function applyJq(payload: ApplyJqPayload) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just move this back into the jq.ts brick module because that's the only place it's called now

Copy link
Contributor

@twschiller twschiller left a comment

Choose a reason for hiding this comment

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

@fregante thanks - see comment about consolidating into the jq.ts file: https://github.com/pixiebrix/pixiebrix-extension/pull/6579/files#r1342959845

@github-actions
Copy link

github-actions bot commented Oct 2, 2023

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
pixiebrix-extension ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2023 6:47am

@fregante fregante enabled auto-merge (squash) October 3, 2023 06:45
@fregante fregante merged commit 28a8c48 into main Oct 3, 2023
11 checks passed
@fregante fregante deleted the F/sandbox/drop-jq-from-sandbox branch October 3, 2023 07:03
@grahamlangford grahamlangford added this to the 1.8.0 milestone Oct 4, 2023
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.

Browser Extension crashes when passing large data to jq (1.7.16-beta.8)
3 participants