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: use legal format for compiler version comment #4637

Closed
wants to merge 6 commits into from

Conversation

cardoso
Copy link
Contributor

@cardoso cardoso commented Oct 14, 2024

Details

Closes #4621

Does this pull request introduce a breaking change?

  • ๐Ÿ˜ฎโ€๐Ÿ’จ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ๐Ÿ”ฌ Yes, it does include an observable change.

In dev mode the compiler version comment emitted will be in the format /*@preserve LWC compiler vX.X.X*/ instead of /*LWC compiler vX.X.X*/. Tools like Vite and Esbuild will now preserve this comment in dev mode by default (configurable in the tools themselves).

GUS work item

@cardoso cardoso requested a review from a team as a code owner October 14, 2024 22:26
@cardoso
Copy link
Contributor Author

cardoso commented Oct 15, 2024

Changed to @preserve and fixed karma tests. There's one failing about noframe but I guess that's related to #4635

Copy link
Contributor Author

@cardoso cardoso left a comment

Choose a reason for hiding this comment

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

@nolanlawson karma tests are passing now, but not sure why I had to update these snapshots...

@@ -4,7 +4,7 @@ import { freezeTemplate, parseFragment, registerTemplate } from "lwc";
const $fragment1 = parseFragment`<xmp${3}>&lt;/xmp&gt;Hello &lt;div&gt;world&lt;/div&gt; <div>foo</div></xmp>`;
const $fragment2 = parseFragment`<iframe${3}>Hello &lt;div&gt;world&lt;/div&gt; <div>foo</div></iframe>`;
const $fragment3 = parseFragment`<noembed${3}>Hello &lt;div&gt;world&lt;/div&gt; <div>foo</div></noembed>`;
const $fragment4 = parseFragment`<noframes${3}><p${3}>It seems your browser does not support frames or is configured to not allow them.</p></noframes>`;
const $fragment4 = parseFragment`<noframes${3}><p>It seems your browser does not support frames or is configured to not allow them.</p></noframes>`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Snapshot update

Comment on lines 217 to 230
"type": "Text",
"raw": "<p>It seems your browser does not support frames or is configured to not allow them.</p>",
"value": {
"type": "Literal",
"value": "<p>It seems your browser does not support frames or is configured to not allow them.</p>"
},
"attributes": [],
"properties": [],
"directives": [],
"listeners": [],
"children": [
{
"type": "Text",
"raw": "It seems your browser does not support frames or is configured to not allow them.",
"value": {
"type": "Literal",
"value": "It seems your browser does not support frames or is configured to not allow them."
},
"location": {
"startLine": 14,
"startColumn": 12,
"endLine": 14,
"endColumn": 93,
"start": 316,
"end": 397
}
}
]
"location": {
"startLine": 13,
"startColumn": 15,
"endLine": 15,
"endColumn": 5,
"start": 304,
"end": 406
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Snapshot update

@nolanlawson
Copy link
Collaborator

I thought about this some more, and I'm not sure this is the right approach anymore:

  1. The original goal of this feature was to stamp version numbers to templates/stylesheets/components in a way that would only be preserved during dev mode. We could have used if (process.env.NODE_ENV !== 'development'), but that would potentially break consumers who aren't ready to support process.env.NODE_ENV (which is currently used in engine-dom but not in the compiled components themselves). Hence we landed on comments.
  2. Adding @preserve means that these comments are preserved in production, which will bloat the bundle size (gzip helps though).
  3. On the Salesforce platform, we could of course add a Rollup plugin to remove @preserve. But this doesn't help off-Salesforce consumers.
  4. This made me wonder though โ€“ could we simply add @preserve using an ESBuild plugin? I assume there is some equivalent of @rollup/plugin-replace in the ESBuild world.

@cardoso
Copy link
Contributor Author

cardoso commented Oct 17, 2024

Thanks @nolanlawson . I'll close this one for now and think about an alternative.

@cardoso cardoso closed this Oct 17, 2024
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.

Use legal format for compiler version comment
2 participants