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: support exportConditions and add worker default conditions #1401

Merged
merged 7 commits into from
Jul 12, 2023

Conversation

Hebilicious
Copy link
Contributor

@Hebilicious Hebilicious commented Jul 6, 2023

πŸ”— Linked issue

Resolves #1371
Resolves #83
Resolves #1123
Resolves nuxt/nuxt#21571

❓ 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 works locally, I tested it against https://github.com/Hebilicious/authjs-nuxt/ which previously needed

      "jose": resolve(__dirname, "../node_modules/jose/dist/browser/index.js"),
      "@panva/hkdf": resolve(__dirname, "../node_modules/@panva/hkdf/dist/web/index.js"),

And now it works πŸŽ‰

image

πŸ“ Checklist

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

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #1401 (fd6e3a6) into main (03540d9) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head fd6e3a6 differs from pull request most recent head 35043a4. Consider uploading reports for the commit 35043a4 to get more accurate results

@@            Coverage Diff             @@
##             main    #1401      +/-   ##
==========================================
+ Coverage   76.73%   76.79%   +0.05%     
==========================================
  Files          71       71              
  Lines        7273     7309      +36     
  Branches      727      729       +2     
==========================================
+ Hits         5581     5613      +32     
- Misses       1691     1695       +4     
  Partials        1        1              
Impacted Files Coverage Ξ”
src/presets/nitro-dev.ts 100.00% <ΓΈ> (ΓΈ)
src/options.ts 94.86% <100.00%> (+0.51%) ⬆️
src/presets/bun.ts 100.00% <100.00%> (ΓΈ)
src/presets/cloudflare-module.ts 100.00% <100.00%> (ΓΈ)
src/presets/cloudflare-pages.ts 58.94% <100.00%> (+0.21%) ⬆️
src/presets/cloudflare.ts 100.00% <100.00%> (ΓΈ)
src/presets/deno-deploy.ts 100.00% <100.00%> (ΓΈ)
src/presets/deno-server.ts 96.15% <100.00%> (+0.02%) ⬆️
src/presets/lagon.ts 50.70% <100.00%> (+0.70%) ⬆️
src/presets/netlify.ts 75.73% <100.00%> (+0.10%) ⬆️
... and 4 more

@Hebilicious Hebilicious requested review from pi0 and danielroe July 6, 2023 17:15
@Hebilicious Hebilicious added bug Something isn't working enhancement New feature or request preset labels Jul 6, 2023
Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Rather than hard coding details about presets in the rollup config, perhaps we could have the export conditions set in the options and specifically in each preset?

That would also make it possible for users to configure this, and keep the core rollup configuration generic.

@Hebilicious
Copy link
Contributor Author

Hebilicious commented Jul 6, 2023

Rather than hard coding details about presets in the rollup config, perhaps we could have the export conditions set in the options and specifically in each preset?

That would also make it possible for users to configure this, and keep the core rollup configuration generic.

@pi0 Regarding nitro.options.dev ? "development" : "production", if I'm not mistaken, this should always be production, except for the "nitro-dev" preset right ? If that's the case I think @danielroe refactor suggestion that implemented in 5194cc7 make things tidy as we can have a static exportConditions for each preset

@pi0 pi0 changed the title feat: add node-resolve export conditions for presets feat: support exportConditions and add platform defaults Jul 12, 2023
@pi0 pi0 changed the title feat: support exportConditions and add platform defaults feat: support exportConditions and add worker default conditions Jul 12, 2023
@pi0 pi0 merged commit f12b38e into main Jul 12, 2023
@pi0 pi0 deleted the fix/rollup-preset-export-conditions branch July 12, 2023 16:56
@pi0 pi0 mentioned this pull request Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request preset ready
Projects
None yet
3 participants