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: add experimental deno-server preset #592

Merged
merged 24 commits into from
Jun 20, 2023
Merged

feat: add experimental deno-server preset #592

merged 24 commits into from
Jun 20, 2023

Conversation

danielroe
Copy link
Collaborator

@danielroe danielroe commented Oct 16, 2022

πŸ”— Linked issue

resolves #422

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR adds a deno preset + deno server entry. You can see it in assembled action in a Nuxt project in https://github.com/danielroe/nuxt-deno - just run pnpm i && pnpm build && pnpm preview.

Questions in my mind:

  • How appropriate is it for us to set version constraints on the deno imports we are generating? Should there be any at all? Should they be generated at build time, if so, or versioned with nitro?
  • Is there a different/better way to consume node imports other than tracing? Could we avail ourselves of a Deno CDN instead?

Thoughts and feedback very welcome @bartlomieju.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codecov
Copy link

codecov bot commented Oct 16, 2022

Codecov Report

Merging #592 (b1abe5c) into main (9bb674f) will increase coverage by 0.34%.
The diff coverage is 96.35%.

❗ Current head b1abe5c differs from pull request most recent head 38b2d6d. Consider uploading reports for the commit 38b2d6d to get more accurate results

@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
+ Coverage   76.22%   76.57%   +0.34%     
==========================================
  Files          68       69       +1     
  Lines        6953     7081     +128     
  Branches      684      705      +21     
==========================================
+ Hits         5300     5422     +122     
- Misses       1652     1657       +5     
- Partials        1        2       +1     
Impacted Files Coverage Ξ”
src/presets/deno-server.ts 96.12% <96.12%> (ΓΈ)
src/presets/deno-deploy.ts 100.00% <100.00%> (ΓΈ)
src/presets/index.ts 100.00% <100.00%> (ΓΈ)
src/rollup/plugins/import-meta.ts 100.00% <100.00%> (ΓΈ)

... and 2 files with indirect coverage changes

src/presets/deno.ts Outdated Show resolved Hide resolved
@pi0
Copy link
Member

pi0 commented Nov 15, 2022

Waiting for refactor of rollup-plugin-deno to unenv and inline impl

@danielroe danielroe marked this pull request as ready for review February 6, 2023 22:26
@danielroe
Copy link
Collaborator Author

danielroe commented Feb 6, 2023

Okay, we're ready to go on deno. Sorry for the delay.

If you want to test with a Nuxt project, make sure to transpile vue or deno will pick the client version when running the project.

// https://nuxt.com/docs/api/configuration/nuxt-config
export default defineNuxtConfig({
  build: {
    transpile: [/@vue/, /\bvue\b/]
  }
})

Note: deno doesn't use the node_modules folder we create, but turning off trace means we include full path in the output. Neither is great. There may be an improvement we can do here to turn tracing off but leave ids unchanged, or alternatively we can leave node_modules in place - it may be possible for deno to respect what we create via https://deno.land/manual@v1.30.2/node/npm_specifiers#--node-modules-dir-flag or similar.

@bartlomieju
Copy link

@danielroe in the next minor release of Deno, coming out in two weeks, Deno should be able to pick up package.json file and use local node_modules/ directory automatically in such case. If you are not in a hurry, I suggest to wait a bit and we can provide some more help to make the integration nicer.

src/presets/deno.ts Outdated Show resolved Hide resolved
src/presets/deno.ts Outdated Show resolved Hide resolved
src/presets/deno.ts Outdated Show resolved Hide resolved
src/runtime/entries/deno-server.ts Outdated Show resolved Hide resolved
src/presets/deno.ts Outdated Show resolved Hide resolved
@pi0
Copy link
Member

pi0 commented Feb 6, 2023

Note: deno doesn't use the node_modules folder we create, but turning off trace means we include full path in the output. Neither is great.

While deno supporting node_modules to enable, can we make it opt-in and use inline strategy? (with chunks)

@danielroe
Copy link
Collaborator Author

While deno supporting node_modules to enable, can we make it opt-in and use inline strategy? (with chunks)

The problem is that then we miss out on the great work with node compat from Deno - so node: imports will fail as they can't be inlined - and we end up needing nodeless environment and any corresponding compatibility issues.

@danielroe
Copy link
Collaborator Author

@bartlomieju No, it's not a bug - there is no index route in the test fixture. Try /api/hello or one of the other routes in test/fixture/api or test/fixture/routes.

@bartlomieju
Copy link

Thanks. I just tried and they all seem to work. Sorry but I'm a bit confused now about the problem here. Is the problem that Deno doesn't install bundleDependencies that are present in package.json?

@danielroe
Copy link
Collaborator Author

The problem is that Deno does install them. Nitro has produced a node_modules folder which is ultra-minimal. It's traced just the files that are used, which results in a hugely decreased server node_modules folder. It also in some cases correctly resolves different versions of packages in a way that works with tree-based node_modules resolution algorithm. Ideally we would like to use this pre-selected node_modules folder. Otherwise it may be (1) larger than needed, which is suboptimal but not a disaster, (2) it may resolve to a single version of a package, when multiple versions may be required by the build - this could be breaking for users.

@danielroe
Copy link
Collaborator Author

@pi0 I know you've approved this previously - is there anything I can do at this point to help get this PR over the line?

@danielroe
Copy link
Collaborator Author

@pi0 Let me know when you are ready to review and I will happily resolve merge conflicts πŸ™

@pi0 pi0 changed the title feat: add deno presets and entries feat: add experimental deno-server preset Jun 20, 2023
@nuxt-studio
Copy link
Contributor

nuxt-studio bot commented Jun 20, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
nitro Edit on Studio β†—οΈŽ View Live Preview 38b2d6d

@pi0
Copy link
Member

pi0 commented Jun 20, 2023

I have made few refactors to keep deno-server implementation fully separated making it easier to move forward also added deno.json with start task and tests.

I think we can move forward since it basically works but there are two important things:

  • Versions are not picked from package.json and externals seem unused (deno downloads them). We might need to add importMaps to the deno.json OR at least add the version to npm: specifier. (local hoist test is skipped since failing)
  • There are some rollup issues with inject plugin. We likely can fix it by reordering again since basic build works seems good to me to move forward.

@bartlomieju
Copy link

@pi0 sorry for a slow response, I missed notitication in my inbox. Is there something we can do on Deno side to make it easier for you folks to handle the problem? Could we somehow limit what is discovered from package.json by Deno or provide a JS library that would allow to leverage node_modules/ structure generated by Deno?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request preset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deno preset
4 participants