epicweb-dev / rocket-rental

Rent Rockets from rocking awesome people
Other
277 stars 34 forks source link

chore: upgrade to remix v1.16.1 and native ESM #12

Closed onemen closed 1 year ago

onemen commented 1 year ago

it works

using serverModuleFormat: 'cjs' at the moment.

once this is merged we can continue and apply the same to kcdshop and all workshops

kentcdodds commented 1 year ago

I need to wait on merging this because there's a bug in Remix's new dev server where it won't serve anything from the public directory other than remix-managed assets.

onemen commented 1 year ago

Ok, I am working on serverModuleFormat: 'esm' part

kentcdodds commented 1 year ago

Sounds good! I think for the purging stuff we'll just use the query string thing and that should be fine.

I'm also definitely interested in seeing whether we can just server restarts faster so we don't have to worry about doing the global stuff everywhere.

onemen commented 1 year ago

@kentcdodds, everything is working, but since it is a big change you better try it before merging to main

"dev": "remix dev -c \"npm run dev:server\"", i did not set --no-restart at the moment but it is also works.

I need to wait on merging this because there's a bug in Remix's new dev server where it won't serve anything from the public directory other than remix-managed assets.

it load the favicon, images and font

I think for the purging stuff we'll just use the query string thing and that should be fine

i don't understand what you refer to with this

I'm also definitely interested in seeing whether we can just server restarts faster so we don't have to worry about doing the global stuff everywhere.

can you point me to the code you like to change

kentcdodds commented 1 year ago

can you point me to the code you like to change

There are two things if I remember right:

  1. Update our prisma connection to use the new experimental json protocol so connections and queries run faster
  2. Improve our event stream for the chat feature so clients reconnect when the server is restarted

Pretty much if we're going to try the server restart method, we don't need (and shouldn't have) anything that uses global with the exception of the ENV thing.

kentcdodds commented 1 year ago

I'm afraid that somehow I pushed conflicts on your PR (I didn't realize you touched those files, sorry!)

onemen commented 1 year ago

@kentcdodds,

Regarding global did you mean we can remove all global references and just do:

// db.server.ts
export const prisma = new PrismaClient()
export const db = new Database(process.env.DATABASE_PATH)

// chat.server.ts
export const chatEmitter = new EventEmitter()

Some issue with serverModuleFormat: 'esm' (both work with serverModuleFormat: 'cjs'):

  1. After I changed npm dev script to include --no-restart "dev": "remix dev -c \"npm run dev:server\" --no-restart", then even after our watcher call broadcastDevReady(build) nothing changed in the UI. i've tried to change title and some labels on root.tsx but nothing changed.
  2. on ESM module require.cache does not have any 'build/' entry so purgeRequireCache don't do anything.

I can make it work with --no-restart by moving the watcher to index.cjs and run

if (process.env.NODE_ENV === 'development') {
    // avoid watching the folder itself, just watch its content
    const watcher = chokidar.watch(`${BUILD_DIR.replace(/\\/g, '/')}/**.*`, {
        ignored: ['**/**.map'],
    })
    watcher.on('all', (...args) => {        
        for (const key in require.cache) {
            if (key.startsWith(BUILD_DIR)) {
                console.log('remove cache', key)
                delete require.cache[key]
            }
        }
        broadcastDevReady(require(BUILD_DIR))
    })
}

then we can keep all global use since we not restarting the server

onemen commented 1 year ago

Is this legitimate method: https://ar.al/2021/02/22/cache-busting-in-node.js-dynamic-esm-imports/

it work like a charm...

if (process.env.NODE_ENV === 'development') {
    // avoid watching the folder itself, just watch its content
    const watcher = chokidar.watch(`${BUILD_DIR.replace(/\\/g, '/')}/**.*`, {
        ignored: ['**/**.map'],
    })
    watcher.on('all', async (...args) => {
        const build = await import(
            `${BUILD_DIR_FILE_URL}/index.js?update=${Date.now()}`
        )
        broadcastDevReady(build)
    })
}
onemen commented 1 year ago

I'm still thinking there's too much annoyance and not enough benefit to going native ESM, so let's not do that yet.

it feel much faster.... with ESM

there is also a fix for https://github.com/remix-run/remix/issues/6271 do you want me to try v0.0.0-nightly-0e2763f-20230503

kentcdodds commented 1 year ago

I believe @pcattori said they're changing things in the near future so the dev server no longer serves assets. I'm hesitant to go full on with this until the dev server is a bit more stable.

kentcdodds commented 1 year ago

Thank you so much!