preactjs / preact-cli

😺 Your next Preact PWA starts in 30 seconds.
MIT License
4.69k stars 375 forks source link

Newest updates to Async Loader cause error message if App component is in /routes #1286

Closed rschristian closed 4 years ago

rschristian commented 4 years ago

Do you want to request a feature or report a bug?

A bug

What is the current behaviour?

Currently the following is written to the console in the browser:

Uncaught Error: Undefined component passed to createElement()

You likely forgot to export your component or might have mixed up default and named imports <#text dangerouslySetInnerHTML="[object Object]" />

As far as I can tell, this does not cause any problems in the app, just the error message on every reload.

If the current behaviour is a bug, please provide the steps to reproduce.

Using the default Preact template, move App.js from /components to /routes. Start the dev server and check the console in your browser.

What is the expected behaviour?

That no error is thrown. If I downgrade to 3.0.0-rc.14 or don't place App.js in /routes everything is fine again.

Please mention other relevant information.

If there's an actual reason why App.js needs to be outside of /routes I can, of course, move it, though it is my personal preference to have it there as I generally think it makes the most sense.

Environment Info:
  System:
    OS: Linux 5.6 Manjaro Linux
    CPU: (4) x64 Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
  Binaries:
    Node: 14.3.0 - /usr/bin/node
    Yarn: 1.22.4 - /usr/bin/yarn
    npm: 6.14.5 - /usr/bin/npm
  Browsers:
    Firefox: 78.0.1
  npmPackages:
    preact: ^10.3.2 => 10.4.5 
    preact-cli: ^3.0.0-rc.16 => 3.0.0-rc.16 
    preact-render-to-string: ^5.1.4 => 5.1.9 
    preact-router: ^3.2.1 => 3.2.1
prateekbh commented 4 years ago

move App.js from /components to /routes

Curious on why would you do this?

rschristian commented 4 years ago

Curious on why would you do this?

Personal taste I guess? Generally, components is for the things I can reuse, routes are for route-specific content.

I’m not going to reuse App.js, so I’ve always moved it into routes. Additionally, having the router next to the routes has always seemed quite logical.

Fair enough if this isn't necessarily meant to be supported and just happened to work without the message before.

developit commented 4 years ago

I would definitely avoid putting App in routes. Routes gets automatically code-splitted, and as a result anything in that directory loads lazily to prioritize rendering the rest of the application. Moving App in there means the whole app loads lazily, which will just make initial rendering slower.

As for the error, I'm guessing you are using a named export for App (h/t @reznord for pointing that out). Named exports are not supported by async-loader, and thus not supported in routes/. It's something we're hopefully going to address, but there are some limitations as a result of the lazy loading stuff that can't be patched up.

In terms of supporting the preference, one option here would be to create a src/pages directory and use that for the things you currently have in src/routes. There's no special behavior attached to a "pages" directory, so no issues with imports. The only change you'll need to make is to manually apply code splitting to your routes since you'd be giving up the automatic splitting from src/routes:

// src/pages/App.js
import Router from 'preact-router';
import Home from 'async!./Home.js';  // the "async!" prefix makes this lazy-loaded

export function App() {
  return (
    <Router>
      <Home path="/" />
    </Router>
  );
}
rschristian commented 4 years ago

Fair enough. I knew the route splitting was there, but as I hadn't seen any issues or (noticeable) slowdowns I just kept it as it seemed to make sense structure-wise. That's not a reason to not move it now though.

The error message does show up using default exports. I know in the past there have been a few threads about using named exports in routes/, such as #757 and #1237, and as such made sure not to use them.

I will probably just move App.js back into components/ but hopefully this will help anyone else who thinks of doing this.

Thanks, appreciate the response.

reznord commented 4 years ago

If you feel that the components are tend to change often but not app.js, then you can always leave it in src/ which won't create any issues 🙃

micahjon commented 2 years ago

For what it's worth, I've found I get this same error if Router is top-level and isn't wrapped in another HTML element (like <div id="preact_root">), e.g.

// app.tsx
const App: FunctionalComponent = () => {
  return (
    <Router>
      <Route path="/" component={Home} />
      <Route path="/profile/" component={Profile} user="me" />
      <Route path="/profile/:user" component={Profile} />
      <NotFoundPage default />
    </Router>
  );
};

I'm fairly new to the React ecosystem, so maybe this is obvious, but figured it'd be worth noting in case others were experiencing the same thing.