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

feat(compiler): officially support Deno #3117

Merged
merged 8 commits into from
May 17, 2022
Merged

feat(compiler): officially support Deno #3117

merged 8 commits into from
May 17, 2022

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented May 6, 2022

Docs

I will update docs/ with mentions to @remix-run/deno as a follow-up PR targeting the main branch since there are changes there that we want to add to.

Testing Strategy

Integration test: deno-compiler-test.ts

Manually testing

Have to jump through a few hoops to get the locally built packages all plumbed through...

  1. Build remix-run/remix:
yarn build
  1. Package @remix-run/deno locally:
cd build/node_modules/@remix-run/deno
npm pack
  1. Create a Remix app with the Deno template, without running npm install:
npx create-remix@latest --template <path/to/templates/deno>
# answer "n" (no) when prompted if `npm install` should be run
  1. Install the locally packaged @remix-run/deno:
npm i <path/to/remix/build/node_modules/@remix-run/deno/package.tar.gz>
  1. Use the local compiler to build, run, and dev
node <path/to/remix/build/node_modules/@remix-run/dev/cli.js> build
node <path/to/remix/build/node_modules/@remix-run/dev/cli.js> start
node <path/to/remix/build/node_modules/@remix-run/dev/cli.js> watch
# in another terminal window...
npm run dev:deno

@pcattori pcattori force-pushed the pedro/deno branch 5 times, most recently from 8c59fae to 98d8696 Compare May 9, 2022 20:01
@pcattori pcattori changed the title Pedro/deno Deno support May 9, 2022
@pcattori pcattori changed the title Deno support feat(deno): officially support Deno May 9, 2022
@pcattori pcattori force-pushed the pedro/deno branch 3 times, most recently from 5e84dc1 to 7d0425b Compare May 9, 2022 20:07
Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

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

This is awesome! 🔥

We'll probably also need magicExports, like we have for cloudflare & node runtimes?

templates/deno/package.json Outdated Show resolved Hide resolved
packages/remix-deno/globals.ts Show resolved Hide resolved
@pcattori
Copy link
Contributor Author

pcattori commented May 9, 2022

We'll probably also need magicExports, like we have for cloudflare & node runtimes?

We don't need nor want magic exports for Deno since we don't have to support existing Deno projects that import {...} from "remix".

@MichaelDeBoey
Copy link
Member

We don't need nor want magic exports for Deno since we don't have to support existing Deno projects that import {...} from "remix".

This would break our "just copy over your app directory to a new project with different adapter/runtime" promise though.

@pcattori
Copy link
Contributor Author

pcattori commented May 9, 2022

We don't need nor want magic exports for Deno since we don't have to support existing Deno projects that import {...} from "remix".

This would break our "just copy over your app directory to a new project with different adapter/runtime" promise though.

We can't promise that. E.g. if someone writes Deno.readFile in their loader, that won't work on Node. A completely portable app/ folder would be ideal, but its not something we currently do.

@MichaelDeBoey
Copy link
Member

We can't promise that. E.g. if someone writes Deno.readFile in their loader, that won't work on Node. A completely portable app/ folder would be ideal, but its not something we currently do.

Good point on that one.

If I recall correctly, thie "advice" @mjackson used to give for changing adapter/runtime has always been "create a new project with the required adapter/runtime & copy over your app folder" (I even thought it was somewhere in the docs, but can't find it), that's why I was thinking the magicExports would be needed

@pcattori pcattori force-pushed the pedro/deno branch 8 times, most recently from 442d954 to d467eb5 Compare May 10, 2022 17:12
integration/deno-compiler-test.ts Outdated Show resolved Hide resolved
integration/deno-compiler-test.ts Outdated Show resolved Hide resolved
integration/deno-compiler-test.ts Show resolved Hide resolved
integration/deno-compiler-test.ts Outdated Show resolved Hide resolved
packages/remix-deno/server.ts Show resolved Hide resolved
templates/deno/README.md Outdated Show resolved Hide resolved
templates/deno/package.json Outdated Show resolved Hide resolved
packages/remix-deno/server.ts Outdated Show resolved Hide resolved
packages/remix-deno/server.ts Outdated Show resolved Hide resolved
packages/remix-deno/server.ts Outdated Show resolved Hide resolved
packages/remix-deno/server.ts Outdated Show resolved Hide resolved
keep url imports as-is in build output so that each runtime can handle url imports in their own way.
for example, deno will download dependencies from url imports at runtime. the browser will download
dependendencies from url imports on-demand. node will simply throw an error when encountering a url
import.

since we will recommend to use npm/node_modules for dependency
management (even for deno), the only url imports we expect are for deno
imports on the server from https://deno.land or from Deno-friendly CDNs
like https://esm.sh .
previously, `shell.grep` was being called with the `-r` option, which it does not recognize,
resulting in an exit ode of 1. we incorrectly interpreted that as `grep` finishing the search
without finding matches. instead, the `shell.grep` call was crashing. now we correctly pass a list
of files to `shell.grep` without the `-r` option.
…ot found

so that `createRequestHandlerWithStaticFiles` is more portable across runtimes and adapters
@pcattori pcattori merged commit aa39050 into dev May 17, 2022
@pcattori pcattori deleted the pedro/deno branch May 17, 2022 18:31
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v1.5.0-pre.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v1.5.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants