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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add app.tsx assets to page event in dev env #1423

Merged
merged 4 commits into from
May 1, 2024

Conversation

katywings
Copy link
Contributor

@katywings katywings commented Mar 27, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • infrastructure changes
  • Other... Please describe:

What is the current behavior?

During development app.css is only loaded in the browser via javascript, resulting in a flash of unstyled content.

What is the new behavior?

app.tsx is registered for asset collection in the pageEvent on the server. This way app.css and other potential css assets of app.tsx are added to the document head, during server side rendering.

Open Question

I am not 100% happy with the inlined src/app.tsx, do we have a proper way to access the path to app.tsx?

Other information

How it currently looks in a browser with disabled js:
IS

How it should look:
SHOULD
owser with disabled js:

Credits

@nksaraf helped me to find the proper place to debug the issue 馃檹.

Copy link

changeset-bot bot commented Mar 27, 2024

馃 Changeset detected

Latest commit: 1a48c63

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@edivados
Copy link
Contributor

edivados commented Mar 29, 2024

Can't reproduce with vinxi 0.3.10 but with vinxi 0.3.11 the issue is there.

With 0.3.10 app.css and dev-overlay/styles.css are present here.

...(await clientManifest.inputs[clientManifest.handler]!.assets()),

I guess this is because of nksaraf/vinxi@04b3ff7 causing start's client/index.jsx to not be processed anymore. I lack insight here... may be intended by vinxi and should now be solved in start.


I am not 100% happy with the inlined src/app.tsx, do we have a proper way to access the path to app.tsx?

src directory (appRoot) is user configurable and jsx extension is also possible.

Suggestions:

Define global constant replacements

define: {
"import.meta.env.START_ISLANDS": JSON.stringify(start.experimental.islands),
"import.meta.env.SSR": JSON.stringify(true),
"import.meta.env.START_SSR": JSON.stringify(start.ssr),
"import.meta.env.START_DEV_OVERLAY": JSON.stringify(start.devOverlay),

...(import.meta.env.DEV ? await clientManifest.inputs[import.meta.env.START_SOME_NAME]!.assets(): []),

Throw it on the ssr router's vite config (this is maybe bad... not sure about vite build mode here)

config("user", {
     ...userConfig,
     start: {
         someName: ""
     }
...(import.meta.env.DEV ? await clientManifest.inputs[serverManifest.dev.server.config.start.someName]!.assets(): []),

@katywings
Copy link
Contributor Author

@nksaraf What do you think about @edivados suggestions?

@nksaraf
Copy link
Member

nksaraf commented Apr 3, 2024

Yeah this suggestion makes sense. The compile time/build time can provide the necessary paths and the runtime can use that.

PalmDevs added a commit to PalmDevs/website that referenced this pull request Apr 27, 2024
@ryansolid ryansolid merged commit 846f5fd into solidjs:main May 1, 2024
1 check passed
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.

None yet

4 participants