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

Fixes app import paths in index.html #58

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

kiwiupover
Copy link
Contributor

This PR fixes the import of file in the index.html file.

Fixes

CleanShot 2024-09-01 at 17 52 21@2x

@@ -18,8 +18,8 @@

<script src="/@embroider/core/vendor.js"></script>
<script type="module">
import Application from './app';
import environment from './config/environment';
import Application from './app/app';
Copy link
Collaborator

Choose a reason for hiding this comment

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

because index.html is in the app folder already, the meaning of this import means that app.js is actually located at <repo>/app/app/app.js -- which is wrong.

I think the change in this PR is papering over a side-effect of the rewritten app still existing.

When observing the output, your change does make sense:
image

However, in source, it does not make sense:
image

so the index.html would need to move to the root of files-override to to have the imports make sense / not be wrong in source.

With the upcoming removal of app rewriting, the proposed change here will make sense if the index.html were moved outside of the app directory.

However (however), the change here also matches what is in the app-template base-app in the embroider repo.

We can move the index.html out of app later.

@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Sep 2, 2024
@NullVoxPopuli NullVoxPopuli merged commit bb4e1d2 into embroider-build:main Sep 2, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Sep 2, 2024
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.

2 participants