Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Update all packages #15

Merged
merged 1 commit into from
Jul 25, 2018
Merged

Update all packages #15

merged 1 commit into from
Jul 25, 2018

Conversation

btd
Copy link
Contributor

@btd btd commented Oct 1, 2017

Hi there.

This PR is pretty optional, but good to have.
My main question, that i tried to solve is to enchace self exclusion for package contents. What i mean exactly: assume such modules injection:

{
  Promise: 'pinkie-promise',
  fetch: 'unfetch'
}

There are several issues with this configuration:

  1. Modules cannot import themselfs. So i would need to add something like this to exclude [ './node_modules/pinkie-promise/**', './node_modules/unfetch/**'].
  2. Polyfills could depend from each other. In my example 'unfetch' depends from 'Promise' polyfill. In this case my exclusion would not allow to replace Promise usage in unfetch to use pinkie-promise.

Currently i solved this with something like this:

const POLYFILLS = {
  Promise: 'pinkie-promise',
  fetch: 'unfetch'
};

const INJECTIONS = Object.entries(POLYFILLS).map(([nativeObj, packageName]) =>
  inject({
    exclude: `./node_modules/${packageName}/**`,
    [nativeObj]: packageName
  })
);

//and later

plugins: [
//...
...INJECTIONS,
//...
]

I have strong filling this plugin should do this itself. Am i right there? I tried to combine PR, but stucked at place where i need to check if id belongs to packageName. Any suggestions there?

@btd
Copy link
Contributor Author

btd commented Nov 17, 2017

ping @Collaborators @Rich-Harris

@btd
Copy link
Contributor Author

btd commented May 31, 2018

Sorry for bothering @lukastaegert or @Rich-Harris can you please take a look. At least acorn update would be nice to have

@MattiasBuelens
Copy link
Contributor

I need to pass a regex as include option, but this is only supported since rollup-pluginutils 2.0.0 whereas this plugin still uses 1.2.0.

I worked around this by forking the repo and then merging this PR myself, but I'd much prefer if this change landed in the "official" plugin. 😃

@lukastaegert
Copy link
Member

Hi @btd, sorry this has been lying dormant for so long. The thing is that due to the very small core team developing rollup, smaller plugins tend to go under the radar easily.

I'll see what I can do.

Another question: Would you be interested in taking over responsibility for this plugin? I could give you the necessary access and publishing rights and you could improve it at your own discretion. I think this would be a win for all of us. Just tell me what you think.

@lukastaegert lukastaegert merged commit ab00920 into rollup:master Jul 25, 2018
@lukastaegert
Copy link
Member

I updated the remaining dependencies and the build/ci scripts as well and published it as 2.1.0.

@btd
Copy link
Contributor Author

btd commented Jul 25, 2018

@lukastaegert yes i am very interesting to taking over this plugin. We are using this plugin everyday.

@lukastaegert
Copy link
Member

Cool, I sent you an invite! This will make you a rollup member and add you to the—newly created—rollup-plugin-inject "team" which should give you full admin access to this repo. You might want to confirm this by e.g. creating and pushing a branch to the repo.

If you tell me your npm handle, I can also give you the necessary publishing rights.

If you need help, a review for any of your work or have any questions, just ping me or use the "request review" feature.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants