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

Run Remix on Deno #2243

Merged
merged 9 commits into from Mar 15, 2022
Merged

Run Remix on Deno #2243

merged 9 commits into from Mar 15, 2022

Conversation

pcattori
Copy link
Contributor

@pcattori pcattori commented Mar 7, 2022

I removed remix-deno package (see commit message for details). Idea is to take the remix-deno package currently inlined in our new Deno template (see https://github.com/pcattori/remix-deno-template for a prototype of that) and make that importable in Deno. Note that this would mean treating remix-deno as a Deno package, not a node package.

I don't think this should be considered a breaking change since (1) Deno support is experimental (2) previous Deno template was immediately erroring out.

Closes: #

  • Docs
  • Tests

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 7, 2022

Hi @pcattori,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@pcattori pcattori force-pushed the pedro/deno branch 3 times, most recently from 692970c to 0e37509 Compare March 7, 2022 16:31
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Mar 7, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@pcattori pcattori changed the title Pedro/deno Run Remix on Deno Mar 7, 2022
@pcattori pcattori changed the base branch from main to dev March 7, 2022 16:33
@pcattori pcattori force-pushed the pedro/deno branch 3 times, most recently from 7d8982d to ece1d63 Compare March 10, 2022 18:05
@pcattori pcattori marked this pull request as ready for review March 10, 2022 22:10
@pcattori pcattori force-pushed the pedro/deno branch 2 times, most recently from 4fe107c to f1421ee Compare March 14, 2022 23:20
"dev:deno": "cross-env NODE_ENV=development deno run --watch --allow-net --allow-read --allow-env ./build/index.js"
},
"devDependencies": {
"@remix-run/dev": "^1.2.2",
Copy link
Contributor Author

@pcattori pcattori Mar 14, 2022

Choose a reason for hiding this comment

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

TODO: We need to update this to be the version for the release of @remix-run/dev that includes these changes. @chaance do we know if this will be 1.2.4 or 1.3.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can say ^1.2.4 and it will work even if we go straight from 1.2.3 to 1.30. Feels a little weird to reference a non-existing version but 🤷 it would work.

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be * for now. The install script finds all remix deps in package.json and updates them with the right version when it uses the template.

Copy link
Member

Choose a reason for hiding this comment

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

@mcansh changed this to actual values in #2189

jacob-ebey and others added 4 commits March 14, 2022 18:32
react is always imported explicilty in deno when authoring react-based jsx (need to double check
this)
Current remix-deno package is being treated like a node package (eslint, jest, tsc, rollup, etc..). It
should instead be a Deno package (i.e. Deno source code, Deno tooling, etc..). Right now we inline a
proper `remix-deno` package in our Deno template, so this remix-deno package is incorrect and
outdated.
@@ -13,6 +13,7 @@

"declaration": true,
"emitDeclarationOnly": true,
"skipLibCheck": true,
Copy link
Contributor Author

@pcattori pcattori Mar 14, 2022

Choose a reason for hiding this comment

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

@jacob-ebey : what was the reason for enabling skipLibCheck? Is it because esbuild-plugin-cache pulls in deno-cache and deno-importmap as dependencies and type checking fails for those?

If so, might be better to create a declaration file for whatever dependencies are causing this issue so that we don't stop type checking our usage of other dependencies with good types.

Copy link
Member

@mjackson mjackson left a comment

Choose a reason for hiding this comment

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

Looking good, @pcattori! Almost there.

"dev:deno": "cross-env NODE_ENV=development deno run --watch --allow-net --allow-read --allow-env ./build/index.js"
},
"devDependencies": {
"@remix-run/dev": "^1.2.2",
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be * for now. The install script finds all remix deps in package.json and updates them with the right version when it uses the template.

templates/deno/app/routes/index.tsx Outdated Show resolved Hide resolved
templates/deno/app/entry.server.tsx Outdated Show resolved Hide resolved
templates/deno/app/root.tsx Outdated Show resolved Hide resolved
pcattori and others added 3 commits March 14, 2022 19:41
temporary scaffoling and known issues described in templates/deno/README.md
Signed-off-by: Logan McAnsh <logan@mcan.sh>
Signed-off-by: Logan McAnsh <logan@mcan.sh>
currently referencing the dev branch, but should reference main branch
for release
@pcattori pcattori merged commit a2427dd into dev Mar 15, 2022
@pcattori pcattori deleted the pedro/deno branch March 15, 2022 01:42
@@ -0,0 +1,6 @@
{
"compilerOptions": {
"lib": ["dom", "dom.iterable", "dom.asynciterable", "deno.ns"]
Copy link
Contributor

@F3n67u F3n67u Mar 22, 2022

Choose a reason for hiding this comment

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

Hi @pcattori could you elaborate on why we should customize lib? this line caused the error reported in #2402

Copy link
Contributor Author

@pcattori pcattori Mar 22, 2022

Choose a reason for hiding this comment

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

I copied it from some of the prior work we had done on Deno, but it might be a vestige from when we were trying to use standard Node tooling + Typescript compiler to produce a Deno bundle. We've since abandoned that approach in favor of writing Deno source code (eventually with Deno tooling like deno fmt, deno lint, etc... in CI).

Could very well be the case that we should remove the lib option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's continue the discussion in #2402

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.

None yet

6 participants