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

fix: adds html injections to dev proxy [CRE-1203] #6686

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

smnh
Copy link
Contributor

@smnh smnh commented Jun 2, 2024

This enables visual-editor plugin to inject snippet into html in dev mode.

Related PRs:

@smnh smnh changed the title fix: adds html injections to dev proxy fix: adds html injections to dev proxy [CRE-1203] Jun 2, 2024
@@ -7,7 +7,6 @@ import { format } from 'util'

import { DefaultLogger, Project } from '@netlify/build-info'
import { NodeFS, NoopLogger } from '@netlify/build-info/node'
// @ts-expect-error TS(7016) FIXME: Could not find a declaration file for module '@net... Remove this comment to see the full error message
import { resolveConfig } from '@netlify/config'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed by this:
netlify/build@cf45931

@@ -1,6 +1,5 @@
import process from 'process'

// @ts-expect-error TS(7016) FIXME: Could not find a declaration file for module '@net... Remove this comment to see the full error message
import { applyMutations } from '@netlify/config'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed by this:
netlify/build@cf45931

Comment on lines 63 to +64
case 'deflate':
return await deflate(body)
return await inflate(body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

inflate is for decompressing.
deflate is for compressing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the case deflate?

Copy link
Contributor

Choose a reason for hiding this comment

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

@smnh I think the PR is ready to merge, just curious about this last point. If case "deflate": return inflate is intentional I think adding a code comment about why we handle the "deflate" case with "inflate" is a good idea here

@smnh smnh force-pushed the create-on-dev-server-CRE-1203 branch from 2655990 to 67a057d Compare June 3, 2024 11:23
@smnh smnh force-pushed the create-on-dev-server-CRE-1203 branch from 67a057d to bd7ed65 Compare June 3, 2024 15:31
Copy link

github-actions bot commented Jun 3, 2024

📊 Benchmark results

Comparing with aba9592

  • Dependency count: 1,228 ⬇️ 0.57% decrease vs. aba9592
  • Package size: 326 MB ⬆️ 9.53% increase vs. aba9592
  • Number of ts-expect-error directives: 978 ⬇️ 1.12% decrease vs. aba9592

@smnh smnh marked this pull request as ready for review June 3, 2024 16:01
@smnh smnh requested a review from a team as a code owner June 3, 2024 16:01
Copy link
Contributor

@TylerBarnes TylerBarnes left a comment

Choose a reason for hiding this comment

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

@smnh I think a small test is a good idea if possible

@smnh smnh requested a review from TylerBarnes June 5, 2024 09:49
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.

None yet

2 participants