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

error instead of warn for missing exports #2157

Open
wants to merge 1 commit into
base: stable
Choose a base branch
from

Conversation

amk221
Copy link

@amk221 amk221 commented Oct 23, 2024

Currently, this:

import { foo } from 'bar';

will cause Webpack to warn if foo does not exist, but this is easily missed. Meaning its possible to ship broken code.

This PR configures strictExportPresence to error instead of warn.

But this is actually deprecated in favour of exportsPresence, which I did try but it wasn't available on the current version.

Also note it will be true by default in v6 of webpack anyway.

Reading:


Regarding tests, since I'm not familiar with the embroider repo I found it hard to know where they should go. I saw some that seemed like an appropriate place, but I'll need help tbh.

@amk221 amk221 force-pushed the strict-export-presence branch from 85f2a3d to a9591be Compare October 24, 2024 08:06
@amk221 amk221 marked this pull request as ready for review October 24, 2024 08:14
@amk221
Copy link
Author

amk221 commented Jan 16, 2025

Anyone?

@NullVoxPopuli
Copy link
Collaborator

sorrry for the delay -- our focus hasn't been on webpack lately.

Looks like CI needs to be re-ran, but the logs are old enough where I can't just click "re-run".

Would you be willing to rebase this PR? sorry!

@amk221 amk221 force-pushed the strict-export-presence branch 7 times, most recently from 32dd502 to 7f7c0fb Compare January 17, 2025 14:38
@amk221
Copy link
Author

amk221 commented Jan 17, 2025

ok.

warning

strictExportPresence is deprecated in favor of exportsPresence option.

Since some time has passed, re-looking into exportsPresence

@amk221 amk221 force-pushed the strict-export-presence branch from 7f7c0fb to 263bfd4 Compare January 17, 2025 15:29
@amk221
Copy link
Author

amk221 commented Jan 17, 2025

Yeh that works, switched from strictExportPresence to exportsPresence

@NullVoxPopuli
Copy link
Collaborator

How should this be framed in the changelog?

  • generally throwing errors where we weren't previously is a breaking change
  • it could be argued that the errors were always there, but now moved from runtime to build time

@amk221
Copy link
Author

amk221 commented Jan 17, 2025

I don't really know. There's some info here, like that it's already true in mjs.

I can see why it would be a breaking change, but, then again if it breaks for somebody then something was already broken they just didn't know it. If they were in runtime, and you weren't actively monitoring in (Sentry for example) you'd never know.

Yeh anyway, I don't necessarily need this to go in, just as a developer it feels like no brainer. Can I leave it to you all to decide whether you want it or not?

Also, what does Vite do?

@NullVoxPopuli
Copy link
Collaborator

Also, what does Vite do?

Vite doesn't let you import things that don't exist 🎉

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.

2 participants