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] package overrides #2358

Closed
wants to merge 12 commits into from
Closed

[feat] package overrides #2358

wants to merge 12 commits into from

Conversation

ignatiusmb
Copy link
Member

Resolves #2242

There's a lot of changes here, but it's mostly refactors and cleanups for reusability.

@changeset-bot
Copy link

changeset-bot bot commented Sep 3, 2021

🦋 Changeset detected

Latest commit: 1c2fb23

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.

The refactorings are good, but I'm not sure about the addition of this. What other use cases for this are there besides the super-duper-niche-case in the referenced issue? Maybe split the refactorings out into a separate PR to get these in first, and then discuss on the addition of the config.

@ignatiusmb
Copy link
Member Author

Good idea, I've opened up a separate PR for that. Other than the one in the issue, I can't think of any right now, tbh.

@ignatiusmb ignatiusmb marked this pull request as draft September 8, 2021 01:17
@benmccann
Copy link
Member

I think a more flexible solution might be to have a function which lets the user mutate the value we compute instead of a key-value map

@ignatiusmb
Copy link
Member Author

We're currently only modifying the exports field from package json, everything else are copied as-is. By using a function, do you mean exposing the entire package.json object for the user to modify?

@benmccann
Copy link
Member

I wasn't necessarily thinking that, but I think I like that solution even better 😄

@ignatiusmb
Copy link
Member Author

Well, it was a combined efforts 😀

@ignatiusmb
Copy link
Member Author

But, as dummdidumm said, I'm not sure what's the use case other than the super-duper-niche referenced issue. I feel it's become a lot more flexible (still not sure about the name override) now that it's a function, but I can't think of an easier way for the user to handle cases like those.

preprocess: preprocess(),
kit: {
package: {
override: (pkg) => ({ ...pkg, svelte: 'index.js' })
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 svelte is being added automatically now, right? So this test seems a bit confusing that we're adding something that is normally already there (or maybe it's updating it?). Perhaps it'd be clearer to just add something foo: 'bar'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this test is based on the issue, which needed to replace the svelte key (src/lib/index.ts -> index.js)

@benmccann
Copy link
Member

This generally seems okay to me. I feel like there will be other use cases we haven't anticipated. But if you don't think it's necessary and prefer to close it that's fine too. What do you think @dummdidumm ?

@dummdidumm
Copy link
Member

I'd say it won't hurt to leave it in draft mode for a few more weeks/months to see if any requests come up that make this a needed feature. But I would definitely not merge it right now.

@netlify
Copy link

netlify bot commented Jan 20, 2022

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: 1c2fb23

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61e9de3c71623300078632f5

@Rich-Harris
Copy link
Member

It's been a few months, what's the status of this? Do we need it or should we close it?

@ignatiusmb
Copy link
Member Author

There hasn't been any activity around the issue itself as well, I guess we don't really need this (I surely don't) and also close the referenced issue as "won't do".

@ignatiusmb ignatiusmb deleted the i2242/package-override branch February 3, 2022 08:19
@jacob-8
Copy link
Contributor

jacob-8 commented May 9, 2022

I'm late to the party, but I have the exact same problem as #2242 in my monorepo. I would like to have:

  • "svelte": "src/lib/index.ts" in my package A package.json so changes are reflected immediately when devving sibling packages of the monorepo that consume package A components
  • However, when packaging and publishing to npm I would like it changed to "svelte": "index.js" in the package dir.

If override: (pkg) => ({ ...pkg, svelte: 'index.js' }) is all the fix that's needed that would be great to have this feature. In the meantime I'll use the following script in my Github action that publishes my package:

import replace from 'replace-in-file';
replace.sync({
  files: 'package/package.json',
  from: 'src/lib/index.ts',
  to: 'index.js',
});

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.

Change "svelte" path in package.json in root during dev and in package dir after npm run package
5 participants