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

[fix] preserve user defined config and files on svelte-kit package #1735

Merged
merged 20 commits into from
Jul 10, 2021
Merged

[fix] preserve user defined config and files on svelte-kit package #1735

merged 20 commits into from
Jul 10, 2021

Conversation

ignatiusmb
Copy link
Member

@ignatiusmb ignatiusmb commented Jun 22, 2021

This PR fixes multiple stuff regarding package cli command

  • Retain original README.md filename casing from project root.
  • Preserve initial package.json and only adds exports map when it's not defined by the user.
  • Force forward slash paths for exports map values

Fixes #1734
Fixes #1856

@changeset-bot
Copy link

changeset-bot bot commented Jun 22, 2021

🦋 Changeset detected

Latest commit: 8f5294f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

this generally seems like a good idea to me, but I might leave this open for a little while to see if Rich had any other concerns in mind that I'm not aware of when he wrote it

@ignatiusmb
Copy link
Member Author

Aha, I found an exception to scripts in package.json keys that we may want to exclude from the final output. It may run some of the lifecycle scripts if defined and most likely ruin the build or publish process. Other than scripts, I think everything else should be safe and have no special stuff.


Sure, I'm also curious for the reasons behind those specific keys. While we're here, I think the only odd one from the changes listed in the description was the "adds exports map when it's not defined by the user", so I'll explain my reasoning in case anyone's curious.

Published npm packages can now "lock" their imports (or exports in this case) to just a certain paths or modules, so there may be a package/internal used only for the modules inside and cannot be directly accessed by the end-user. There's also a reason for authors to manually specify the exports, be it only exposing some of them or adding { "browser": ... } and such. svelte-kit package forcefully overwrites this and authors would need to manually update the generated package.json again. I think the easiest thing to do without adding another config option is to just check the existence and use the built-in exports map when there's none defined.

@dummdidumm
Copy link
Member

In #1646 I added logic for adding types to the package.json . If this was merged, should I revert the part about "define in your svelte-config.js what the types value is? I guess it would make sense to remove it.

@ignatiusmb
Copy link
Member Author

I think it's a good addition for the same reason some authors might not manually specify them. Perhaps only add types and use the defined config when none is defined in the original package.json too, just like exports here?

@benmccann benmccann changed the title preserve user defined config and files on svelte-kit package [fix] preserve user defined config and files on svelte-kit package Jul 3, 2021
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could you add a test verifying the new behavior? I recently did set up the infrastructure to test this area.

@ignatiusmb
Copy link
Member Author

ignatiusmb commented Jul 8, 2021

Added and adjusted the tests to also handle nested files.

I haven't added the test for the README copying, I think I prefer #1747 more. But, in case this is merged before the other, it'll still solve the initial problem in #1734, so I won't revert the changes for now

Edit: I've added test for file preservation as well, which was one of the main point of this PR.

@dummdidumm
Copy link
Member

Let's first try to get #1747 in and then adjust this one.

@ignatiusmb
Copy link
Member Author

Thanks @dummdidumm 😄, finally got it working now

@benmccann
Copy link
Member

I merged #1747, so you can rebase this one now

@ignatiusmb ignatiusmb marked this pull request as draft July 9, 2021 23:29
@ignatiusmb ignatiusmb marked this pull request as ready for review July 9, 2021 23:41
@benmccann benmccann merged commit 868f97a into sveltejs:master Jul 10, 2021
@ignatiusmb ignatiusmb deleted the kit/package-preservation branch July 10, 2021 13:23
sidharthv96 added a commit to sidharthv96/kit that referenced this pull request Jul 11, 2021
* 'master' of github.com:sidharthv96/kit: (1114 commits)
  Version Packages (next) (sveltejs#1858)
  Bump vite-plugin-svelte to 1.0.0-next.12 (sveltejs#1869)
  [fix] preserve user defined config and files on `svelte-kit package` (sveltejs#1735)
  [fix] handle undefined body on endpoint output (sveltejs#1808)
  [fix] copy essentials files from root on packaging (sveltejs#1747)
  [docs] sort config alphabetically (sveltejs#1867)
  add config.kit.package.emitTypes option (sveltejs#1852)
  [fix] add $lib alias to js/tsconfig (sveltejs#1860)
  Pass along custom properties added to Error (sveltejs#1821)
  Version Packages (next) (sveltejs#1840)
  Improve grammar in packages FAQ
  Docs for writing an adapter (sveltejs#1846)
  Additional documentation around pnpx changeset usage
  [feat] expose Vite.js `mode` from `$app/env` (sveltejs#1789)
  Service worker files exclusion support (sveltejs#1645)
  chore: Enable `vite.server.fs.strict` internally by default (sveltejs#1842)
  Test with the latest version of Svelte (sveltejs#1848)
  [docs] don't need to run pnpm install twice
  Improve HN example docs
  [fix] correct `ReadOnlyFormData` generator implementation (sveltejs#1837)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default export from svelte-kit package svelte-kit package retain original casing in file names
3 participants