Skip to content

Conversation

@DemianX0
Copy link

@DemianX0 DemianX0 commented Mar 15, 2020

@MaxSem
Copy link
Contributor

MaxSem commented Mar 15, 2020

No tests?

@Krinkle
Copy link
Member

Krinkle commented Mar 18, 2020

This is good for compat in general, but I think our system would remain easier to reason about if we don't allow variable imports. I'm not aware of a use case for it currently in MediaWiki, and such constructs tends to make for hard-to-maintain or buggy code based on past experiences.

If we accept this, I would recommend that initially our LESS linter would disallow use of it in MW-related code.

@DemianX0
Copy link
Author

I'm not aware of a use case for it currently in MediaWiki

Use-case:
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/580558/3/resources/src/mediawiki.less/mediawiki.theme/variables.less

I think our system would remain easier to reason about if we don't allow variable imports.

Variable imports are uncommon, so far nobody felt the need for it. The very few cases (1 now, probably for ever) can be done safely with good documentation. The explicit nature of this import and simplicity of LESS makes it easier to understand and reason about than a custom (proprietary) solution in ResourceLoader that potentially requires reading pages of PHP code to find.

If we accept this, I would recommend that initially our LESS linter would disallow use of it in MW-related code.

Good idea. I don't think there'll be ever more than one usage of this LESS feature.

Copy link
Member

@Krinkle Krinkle left a comment

Choose a reason for hiding this comment

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

Holding back for now. The skin variable project (for MediaWiki core) is not yet on the annual roadmap. When it is, this will be considered as part of that.

@DemianX0
Copy link
Author

Holding back for now. The skin variable project is not yet on the annual roadmap. When it is, this will be considered as part of that.

I don't think such delay is necessary or beneficial. It takes away from the momentum.

@dleffler
Copy link

Isn't this what is done with Twitter Bootstrap 3, especially with boot swatches?

@DemianX0
Copy link
Author

Isn't this what is done with Twitter Bootstrap 3, especially with boot swatches?

It could be used to parameterize which swatch to load.

@DemianX0
Copy link
Author

Holding back for now. The skin variable project is not yet on the annual roadmap. When it is, this will be considered as part of that.

This is a fix for a bug (incompatibility with less.js) that would benefit third party users as well.
It is also a prerequisite for my POC: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/580558
which is one solution to https://phabricator.wikimedia.org/T112747 that the WMF failed to solve for 5 years.
I understand Krinkle does not like that solution, but that's not a technical concern, nor a reason to block demonstrating a solution.

After 3 months of nothing happening in these areas, do you wish to hold this up more, or can we consider making some actual improvement and doing some collaboration?

Copy link
Member

@Krinkle Krinkle left a comment

Choose a reason for hiding this comment

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

Needs tests.

@DemianX0 DemianX0 closed this Jul 9, 2021
@DemianX0 DemianX0 deleted the fix-variables-in-import-statement branch July 9, 2021 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants