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

[RFC] Provide unpolyfill meta-package #225

Closed
jderusse opened this issue Jan 30, 2020 · 13 comments
Closed

[RFC] Provide unpolyfill meta-package #225

jderusse opened this issue Jan 30, 2020 · 13 comments

Comments

@jderusse
Copy link
Member

jderusse commented Jan 30, 2020

When extension is installed, the related polyfill is not need, but is still downloaded + bootstraped.

The idea is to provide composer meta-package (without code) to avoid that

{
  "name": "symfony/not-polyfill-intl",
  "require": {
    "ext-intl": "*"
  },
  "replace": {
    "symfony/polyfill-intl-grapheme": "*",
    "symfony/polyfill-intl-icu": "*",
    "symfony/polyfill-intl-idn": "*",
    "symfony/polyfill-intl-normalizer": "*",
  }
}
{
  "name": "symfony/not-polyfill-php73",
  "require": {
    "php": ">=7.3"
  },
  "replace": {
    "symfony/polyfill-php73": "*",
    "symfony/polyfill-php72": "*",
    "symfony/polyfill-php71": "*",
    "symfony/polyfill-php70": "*",
    "symfony/polyfill-php56": "*"
  }
}

users can just composer req symfony/not-polyfill-php73 to remove those un-needed packages.

WDYT?

@javiereguiluz
Copy link
Member

@jderusse even if your proposal would be useful, I think this is something that should be solved on Composer side. We need an option to tell Composer: "ignore this package completely if ...." and the conditions are: PHP version and/or PHP constraints. E.g.:

{
  "name": "symfony/polyfill-intl-icu",
  "ignore-if": {
    "ext-intl": "*"
  },
  "...": "..."
}
{
  "name": "symfony/polyfill-php72",
  "ignore-if": {
    "php": "^7.2.0"
  },
  "...": "..."
}

@IonBazan
Copy link
Contributor

There is a feature request: composer/composer#7557 that would solve this issue. So far nobody provided a PR for this.
You may also check https://github.com/BackEndTea/un-poly-all.

@stof
Copy link
Member

stof commented Jan 30, 2020

@javiereguiluz the issue with that on the composer side is that the lock file would then depend on this ignore-if matching or no, making it more complex to have a portable one (if you have ext-intl installed, the package would be ignored, and so its dependencies should also not be installed, but then the lock file is unusable for someone not having ext-intl)

@stof
Copy link
Member

stof commented Jan 30, 2020

To handle extension polyfills properly (php version polyfills would not be solved by that idea), the cleaner solution would actually be to solve the issue with packages providing extensions (see composer/composer#5030, which might be solved in Composer 2 due to the refactoring of the solver btw, but I haven't verified it).
Then, packages could require ext-intl rather than symfony/polyfill-intl-icu, and projects could require a polyfill package if they want to use it to fulfill the extension requirement (we would probably have to combine out intl polyfill packages into one, as that's a single PHP extension).

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 30, 2020

We could have flex handle this for us: when the extension is here, it would add it to the composer.lock file and it would remove the corresponding polyfills.
Doing it reverse wouldn't work with partial polyfills I fear (eg ext-intl is not fully polyfilled).

PR welcome on flex :)

@jderusse
Copy link
Member Author

hmm.. it requires using flex, moreover how flex knows that a polyfill can be removed? Because an ext appear in the "suggest"? Or because it match "/polyfill-" or "/compat-".?//

@stof
Copy link
Member

stof commented Jan 30, 2020

Altering the lock file is very risky, as composer install with a lock file still runs the solver (with only the lock file as input for packages), and it could break things.
Thus, to be safe, we would also need a requirement on the extension (otherwise, nothing would forbid running composer install without the extension and with the polyfill gone).
The current solution based on requiring the extension and replacing polyfill packages is much safer than altering the output of the solver after it ran.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 30, 2020

hmm.. it requires using flex

It requires using flex, yes, no issues here, that's what plugins are for, enhancing composer.

moreover how flex knows that a polyfill can be removed?

Because we know the rules and we can code them into it. It's not like new polyfills popup everyday. That's a very finite domain.

altering the output of the solver after it ran

I meant before running the solver, in a way that would make composer scream if you "composer install" without the extension while flex made composer take it into account. I think we have the same constraints in mind :)

@Seldaek
Copy link
Member

Seldaek commented Jan 31, 2020

IMO in Composer 2.0 it would be possible to do something like @javiereguiluz suggested, a new field on packages like "ignore-if": {"php": "^7.2", "ext-intl": "*"}, except this would not ignore the package entirely, it would still end up in the lock file, it would just be ignored at install time.

I can't guarantee 100% that this works but I believe it's much more doable now as the lock file construction/composer update is not looking at the installed.json anymore.

The benefit is that it is in the lock file, so depending on the real PHP version you execute with it would be installed or not (I think platform config should be ignored for this). Now obviously the problem is that as we saw with the php version being different at runtime than at composer install time, people do crazy things sometimes. So this probably would end up breaking in some places where people would run install with a newer php than they have in prod and then the polyfill would be missing.

Not sure if it's worth all this trouble and potential issues just to save a few if(extension_loaded()) at runtime?

@BackEndTea
Copy link
Contributor

For what its worth, an idea proposed in #138 was to have a version 9.99.99 that would require the extension and not include any code.

So users could require ^1.0 || 9.99.99. Where composer would install the higher version if it is not available.
That is how random_compat does things.

@jderusse
Copy link
Member Author

jderusse commented Feb 5, 2020

that won't work if you require a library that requires ^1.0

@BackEndTea
Copy link
Contributor

True, its also a chance to open a PR to that package to change that.

Its not the perfect solution, and that is one of the reasons the PR wasn't merged. But it is one of the few options we have with current tooling.

@nicolas-grekas
Copy link
Member

I'm closing here as this repo won't provide the solution.
Note that creating a metapackage that "replaces" some polyfills doesn't work as seamlessly as we would wish: if your root composer package already replaces a polyfill, adding this metapackage that also replaces the same polyfill makes the solver report a failure.

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

No branches or pull requests

7 participants