QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.81k stars 1.3k forks source link

[✨] Remove this warning: `routeLoader$()` can only be declared in `layout.tsx`, `index.tsx` and `plugin.tsx` inside the src/routes directory #5460

Closed Nefcanto closed 11 months ago

Nefcanto commented 11 months ago

Is your feature request related to a problem?

Yes,

we want to reuse some routeLoader$ functions across multiple routes, and in many projects.

This means that we create a file called Loader.jsx and have a routeLoader$ in it. Yet it's not used directly. It exports the function, and then we import it in another place:

For example, this is our BlogLoader.jsx file:

import { routeLoader$ } from '@builder.io/qwik-city'
import {
    getFromCacheOrApi,
    useAsync
} from 'Base'
import { getLayout } from 'Contents'
import { getForm } from 'Forms'
import { getGlobalization } from 'Globalization'

const loadPost = routeLoader$(async (props) => {
    const {
        params,
        status,
        query
    } = props
    const { slug } = params || {}
    const locale = query?.get('locale')
    const [
        data,
        layout,
        globalization,
        form
    ] = await useAsync([
        getFromCacheOrApi(`/blogPost/get?slug=${slug}&locale=${locale}`),
        getLayout('post', props),
        getGlobalization(props),
        getForm(`comment`, props)
    ])
    if (data.error?.code === "404") {
        return status(404)
    }
    if (data.statusCode) {
        return status(data.statusCode)
    }
    const { content, ...contentLessForm } = form
    return {
        ...data,
        ...layout,
        ...globalization,
        ...contentLessForm
    }
})

export default loadPost

This loader can be used in blog/index.jsx or artciles/index.js or news/index.jsx. For example, this is our blog/index.jsx in our Blog module:

import { component$ } from '@builder.io/qwik'
import { NotFound } from 'Base'
import { useSeo } from 'Seo'
import {
    Layout,
    loadBlog,
} from 'Blog'
import { loadBlog as runnableLoader } from 'Loaders'
import { Layout as RunnableLayout } from 'BlogParts'

const Blog = component$(() => {

    const data = loadBlog().value
    const extraData = runnableLoader().value
    if (data === 404) {
        return <NotFound />
    }

    const {
        categories,
        latest,
        mostViewed,
        popular,
        posts,
        seo,
        tags,
    } = data || {}

    return RunnableLayout
        ?
        <RunnableLayout {...data} />
        :
        <Layout {...data} />
})

export default Blog

export { loadBlog }

export { runnableLoader }

const head = ({ resolveValue }) => {
    return useSeo(loadBlog, resolveValue)
}

export { head }

It works just great. It utilizes the very fundamental import/export capability of JS and yet when we develop our project or build it and deploy it we see tons of warnings complaining that:

vite-plugin-qwik: routeLoader$() can only be declared in layout.tsx, index.tsx and plugin.tsx inside the src/routes directory, instead it was declared in "/Holism/SiteQwik/src/Modules/Products/Loaders/LoadProducts.jsx". Please check the docs: https://qwik.builder.io/docs/route-loader/

Describe the solution you'd like

Please remove this warning. You are being very very opinionated here. Let teams decide about the framework and architecture for themselves.

Describe alternatives you've considered

We could not find an alternative. We just suffer by tons of warnings, while we do valid JS. And Qwik is just a framework on JS, thus it should accept JS-valid stuff.

Additional context

No response

maiieul commented 11 months ago

I think this warning is here to warn the developer that route loaders have to be called within a route and cannot be used at the component level (for waterfall reasons if my understanding is correct).

Maybe instead of

routeLoader$() can only be declared in layout.tsx, index.tsx and plugin.tsx inside the src/routes directory, instead it was declared in "/Holism/SiteQwik/src/Modules/Products/Loaders/LoadProducts.jsx".

We could implement an eslint rule that fires when a loader is being called in a component:

routeLoader$() hooks can only be called in layout.tsx, index.tsx and plugin.tsx inside the src/routes directory, instead it was called in "/Holism/SiteQwik/src/Modules/Products/Loaders/LoadProducts.jsx".


In the meantime, you can workaround this by disabling this warning in your eslintrc file with the following rule:

    "qwik/loader-location": "off",
Nefcanto commented 11 months ago

I added that rule to the .eslintrc.cjs and it did not work. This is the content of that file:

module.exports = {
  root: true,
  env: {
    browser: true,
    es2021: true,
    node: true,
  },
  extends: [
    'eslint:recommended',
    'plugin:@typescript-eslint/recommended',
    'plugin:qwik/recommended',
  ],
  parser: '@typescript-eslint/parser',
  parserOptions: {
    tsconfigRootDir: __dirname,
    project: ['./tsconfig.json'],
    ecmaVersion: 2021,
    sourceType: 'module',
    ecmaFeatures: {
      jsx: true,
    },
  },
  plugins: ['@typescript-eslint'],
  rules: {
    '@typescript-eslint/no-explicit-any': 'off',
    '@typescript-eslint/explicit-module-boundary-types': 'off',
    '@typescript-eslint/no-inferrable-types': 'off',
    '@typescript-eslint/no-non-null-assertion': 'off',
    '@typescript-eslint/no-empty-interface': 'off',
    '@typescript-eslint/no-namespace': 'off',
    '@typescript-eslint/no-empty-function': 'off',
    '@typescript-eslint/no-this-alias': 'off',
    '@typescript-eslint/ban-types': 'off',
    '@typescript-eslint/ban-ts-comment': 'off',
    'prefer-spread': 'off',
    'no-case-declarations': 'off',
    'no-console': 'off',
    '@typescript-eslint/no-unused-vars': ['error'],
    '@typescript-eslint/consistent-type-imports': 'warn',
    'qwik/loader-location': 'off',
  },
};

I added it to rules node, last line.

maiieul commented 11 months ago

Tested this one more time on another repo and it always works on my end.

Looking further into this, this warning definitely comes from the eslint rule as it is the only place where you can find it in the Qwik codebase.

Do you still have the warning in development? You should have it printed out in your IDE even when using jsx.

You could try removing "plugin:qwik/recommended" if you can't see eslint warnings (just to see if your errors disappear).

Nefcanto commented 11 months ago

@maiieul even after removing the plugin:qwik/recommended we get those warnings. We're developing inside a docker container, thus we don't see or use eslint during the development in our VS Codes. We only see those warning when we first setup our projects, and then when we want to build it.

maiieul commented 11 months ago

Since the warning comes from the eslint plugin, it means the eslint config is being used when you setup your projects and at build time. You probably need to find a way to disable it in there as well.

maiieul commented 11 months ago

Another option could be to move your reusable loaders out of your source directory. The eslint config is applied based on the tsconfig on new starter projects.

Nefcanto commented 11 months ago

This is so frustrating. What we do is valid JS. We export/import parts of our code to make our code cleaner more reusable and readable. Qwik team should not prevent valid JS techniques.

mhevery commented 11 months ago

My suggestion is to use eslint comment to disable the warning if you know what you are doing.

Nefcanto commented 11 months ago

@mhevery, We added rules to eslint, but it had no effect. How should we disable it? What code should we add to eslint? Also, my feedback is that it's too opinionated not to let a loader be defined out of layout or index and be reused across many files. It's normal JS. Please reconsider this decision.

maiieul commented 11 months ago

@Nefcanto you need to add a comment on top of the file where this warning gets generated.

/* eslint-disable qwik/loader-location */

This issue is related to #5555 .

Nefcanto commented 11 months ago

@maiieul, that's highly unproductive. Do you mean we have to add that repetitive file everywhere? That's very bad.

maiieul commented 11 months ago

@Nefcanto I know. This is a workaround for now.

Nefcanto commented 11 months ago

@maiieul, thank you, but because we have many, many files in our modules, we literally can't ask our developers to put that in every file they have. May I ask what prevents us from configuring eslint centrally?

gioboa commented 11 months ago

You can define "qwik/loader-location": "off" in your .eslintrc config. By the way, sometimes you need to restart VSCode a few times to get eslint to work properly

Nefcanto commented 11 months ago

@gioboa as I have mentioned above, I added it, and it did not work. I'll try again.

gioboa commented 11 months ago

I tried with a fresh 1.3.0 project too and it's working 🤷🏼‍♂️

Nefcanto commented 11 months ago

@gioboa this is an actual repository of our current code. It's not an MRE, yet testing it is not difficult.

You should see tons of them.

maiieul commented 11 months ago

@gioboa, I think your PR solves this issue.

@Nefcanto, could you test this with

    "@builder.io/qwik": "github:BuilderIO/qwik-build#f222b8fe29b078b9696a39310db26b9f45bb0c3d",

(and npm/pnpm install)

:pray: