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

Rename strict flag to strictMode #1383

Merged
merged 6 commits into from
Apr 11, 2023
Merged

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Mar 28, 2023

Turns out we've had a discrepancy between strict and strictMode 🙃

There is another option to rename all the strictMode things to strict, but I think this would be more disruptive.

@NullVoxPopuli NullVoxPopuli force-pushed the fix-1382 branch 2 times, most recently from 88ec912 to ca451aa Compare March 28, 2023 16:34
@NullVoxPopuli NullVoxPopuli changed the title Attempt failing test for ensureSafeComponent Add test coverage for ensureSafeComponent in strict mode Mar 28, 2023
@ef4
Copy link
Contributor

ef4 commented Mar 28, 2023

This seems like potentially a good bug fix but the way you're describing and testing it here is deeply misleading.

There is never, ever a reason to use ensure-safe-component in strict mode.

The existing leaves strict mode templates alone test is all you need. If it turns out it's really supposed to use strictMode instead of strict, then that test can be updated to be spelled that way. But other tests beyond that are just testing the same thing in a weird and misleading way.

@NullVoxPopuli
Copy link
Collaborator Author

If it turns out it's really supposed to use strictMode instead of strict,

yeah, I've discovered that the RFC was the only one using strict -- I have an update to that here: emberjs/rfcs#920

@NullVoxPopuli NullVoxPopuli changed the title Add test coverage for ensureSafeComponent in strict mode Rename strict to strictMode Mar 28, 2023
@simonihmig
Copy link
Collaborator

For the record: I just synced with Preston, as I also came to the conclusion when debugging #1382 that the reason for this was this mix up of the name of the strict mode flag. In the addon's dist output I had strictMode, and as you explained the component helper should not even try to resolve its argument and complain about dynamic usage. But it did. And by patching resolver-transform.js and renaming strict to strictMode as in this PR the error was gone, and Embroider was correctly identifying the compiled template tag coming from the addon as a strict mode template, bypassing all those resolver checks!

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review March 28, 2023 19:37
@NullVoxPopuli NullVoxPopuli changed the title Rename strict to strictMode Rename strict flag to strictMode Apr 4, 2023
@ef4 ef4 merged commit a993025 into embroider-build:main Apr 11, 2023
This was referenced May 2, 2023
@ef4 ef4 added the bug Something isn't working label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants