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 SSR fixtures to emit individually transformed files #4835

Open
wjhsf opened this issue Nov 12, 2024 · 6 comments
Open

Update SSR fixtures to emit individually transformed files #4835

wjhsf opened this issue Nov 12, 2024 · 6 comments
Labels
Up for grabs Issues that are relatively small, self-contained, and ready for implementation

Comments

@wjhsf
Copy link
Contributor

wjhsf commented Nov 12, 2024

Currently, the SSR fixtures emit a single file, compiled-experimental-ssr.js, which is generated by rollup. This can hide some issues, such as unused imports, that would be more easily identified if the fixtures were generated as individual files prior to bundling into a single file.

This will require a bit of additional logic to fully walk the import graph to identify all files that may need to be transformed.

@wjhsf wjhsf added the Up for grabs Issues that are relatively small, self-contained, and ready for implementation label Nov 12, 2024
@nolanlawson
Copy link
Collaborator

We already have es-module-lexer which can be used for this.

@cardoso
Copy link
Contributor

cardoso commented Nov 13, 2024

There's no way as far as I know to make rollup keep unused imports. Perhaps this should be done without rollup?

@nolanlawson
Copy link
Collaborator

Yeah the idea is:

  1. Use es-module-lexer to crawl the module graph
  2. Run @lwc/compiler directly on each file instead of @lwc/rollup-plugin

There is a case to be made that this isn't worth it – we may end up writing a lot of extra test code just for a bit of extra debuggability.

@wjhsf
Copy link
Contributor Author

wjhsf commented Nov 13, 2024

There is a case to be made that this isn't worth it – we may end up writing a lot of extra test code just for a bit of extra debuggability.

We can't test that #4840 works without some way of generating non-bundled files.

@nolanlawson
Copy link
Collaborator

I don't understand. Don't we have tests here to confirm that there are no unused import warnings?

onwarn({ message, code }) {
if (
code === 'CIRCULAR_DEPENDENCY' ||
// TODO [#4793]: fix unused imports
code === 'UNUSED_EXTERNAL_IMPORT'
) {
return;
}
throw new Error(message);
},

@wjhsf
Copy link
Contributor Author

wjhsf commented Nov 13, 2024

Oh yeah, once we fix all unused imports, then we can delete line 64 and it'll be implicitly tested. We just got nothing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Up for grabs Issues that are relatively small, self-contained, and ready for implementation
Projects
None yet
Development

No branches or pull requests

3 participants