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] merge packaging exports #2327

Merged
merged 12 commits into from
Sep 3, 2021
Merged

Conversation

ignatiusmb
Copy link
Member

Resolves #1944

Still not sure if package.exports.behavior is a good settings, inputs are welcome. Also note to self, need to add some docs.

@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2021

🦋 Changeset detected

Latest commit: dce75bd

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

@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.

I think this is a good addition. I'm split on whether we should do a boolean (that could become a function) or do the enum. If there's more than true/false/function, then definitely the enum, but other than deepMerge I don't see a valid third option (and maybe merge should merge deep anyway).

packages/kit/src/packaging/index.js Outdated Show resolved Hide resolved
packages/kit/src/packaging/index.js Outdated Show resolved Hide resolved
@ignatiusmb
Copy link
Member Author

Come to think of it, there can't be a situation where both behavior is set to replace and include/exclude are also used. What if we set the default value of kit.package.exports to null, so exports map in package.json wouldn't get tampered by SvelteKit when user has defined it by themselves, but if it's empty, it will use the generated version.

One awkward thing to this is that users that would like to get their self-defined exports map merged but still have all other exports generated would need to pass an empty object to the config, exports: {}, to indicate they're not replacing the generated ones. But, this allows us to keep the config as is, any thoughts @benmccann @dummdidumm?

@dummdidumm
Copy link
Member

I'm not completely sure I understand. Rereading #1944, there's truth in the OP's post: " if I don't want svelte-kit package to touch my exports I can just do exports:{ exclude: ["**"] }." So maybe the best way would be to just skip the extra layer of "is this null" or "add a behavior param" and instead request users to configure their include/exclude correctly. The new default behavior would be to merge everything present in package.json - why else would you add exports there if you want them overwritten?

@ignatiusmb
Copy link
Member Author

What would I do without you and your reviews @dummdidumm

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.

Well I didn't get to this in hindsight obvious conclusion, too, at first 😄

@ignatiusmb ignatiusmb merged commit fe642d2 into master Sep 3, 2021
@ignatiusmb ignatiusmb deleted the i1944/package-exports-config branch September 3, 2021 13:02
@github-actions github-actions bot mentioned this pull request Sep 3, 2021
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.

A way append to svelte-kit package's exports output.
3 participants