-
Notifications
You must be signed in to change notification settings - Fork 143
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 @embroider/core/entrypoint to @embroider/virtual/compat-modules #2131
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to approve this since it does what it says on the tin and solves a serious type issue that we had. I wonder though if there could be more discussion about the approach and see if we can't do something a little smarter rather than having another package to deploy 🙈 (see my question in a comment)
@@ -17,7 +17,7 @@ | |||
{{content-for "body"}} | |||
|
|||
<script src="/@embroider/core/vendor.js"></script> | |||
<script src="/@embroider/core/entrypoint" type="module"></script> | |||
<script src="/@embroider/virtual/compat-modules" type="module"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will literally do nothing 🤔 are these tests even run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to be updated than to lie?
🤷🙈🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean if you delete the line no test will fail, if you delete the file no test will probably fail 🤷
there is no side-effect of importing /@embroider/virtual/compat-modules
any more so just adding a script will do nothing to any app ever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we delete the whole dummy folder?
I can't leave @embroider/core/entrypoint because it doesn't exist anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like these tests are ran https://github.com/embroider-build/embroider/actions/runs/11151185253/job/30994094350?pr=2138#step:5:293
I think we should do the same thing vite does for extra types ( They tell you to put this in your tsconfig:
For us that could be:
And I think then we just need some |
Link to their docs: https://vitejs.dev/guide/features.html#client-types |
Possibly it would be safer for us to pick a subpath like |
@ef4 what about |
I'm not sure if everything we'll ever want to put in there is under |
As I was poking around in here , I noticed that test-support and vendor are also virtual files -- should those be under the same prefix fake-package? (maybe in a separate PR, once we work out the mechanics here?) |
everything that is virtual should be renamed as such, and yes they should be separate PRs 👍 |
bac31d9
to
ac242d3
Compare
this is also the same thing we do :P 🎉 |
@ef4 I've updated this PR to
But used Additionally, I think I found a bug with TS where @-prefixed libraries are not resolvable from |
@@ -1,3 +1,4 @@ | |||
/// <reference types="@embroider/core/virtual" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the only way I could get the types present when type-checking.
The (imo) nicer way, via compilerOptions.types
doesn't seem to work for @-prefixed package names.
513040c
to
701da5c
Compare
701da5c
to
5fe5999
Compare
… types Remove unneeded types-inclusion
5fe5999
to
9fe67fc
Compare
Resolves #2025