-
Notifications
You must be signed in to change notification settings - Fork 86
Description
Motivation
Currently, the Compiler class is huge (approx 9kLOC) which makes it harder to work on them:
- identifying the public API is hard
- such big file slows down IDE (my PHPStorm regularly gets stuck at analyzing the code while I type when working in this file, which then prevents using autocompletion)
The current architecture also suffers from some weaknesses:
- the discovery of available functions and their prototype is magic
- all these
lib*methods are considered unused by static analysis, because it cannot recognize the way they are used - overloaded functions (functions with multiple prototypes) have to guess the prototype being used based on the arguments, while the calling code already knows exactly which overload was used
- this architecture would not play well with implementing modules
Proposal
SassScript functions are moved into a bunch of separate (internal) classes in the ScssPhp\ScssPhp\Functions namespace. These functions are grouped by Sass module to which they related (so this namespace would contain Color, Math, List, Map, String, Selector and Meta classes, with a suffix to decide upon to avoid issues as we have 2 reserved keywords in that list of names). Functions will be implemented as public static methods in these classes. Overloads will be able to use separate callables to simplify the implementation. Helper functions will become private static methods in the classes using them, or be moved to a separate Util or to the Value API if they need to be reused across modules.
Some meta functions will still be implemented in the compiler layer because they require access to internals of the evaluation environment (that's what they are meant to expose). For reference, dart-sass also treat them in a special way for that reason. In practice, I think they will use an infrastructure similar to the one of custom functions. Some meta functions can be implemented standalone and will follow the normal pattern though.
Then, for the registration of functions with their signatures, I suggest to have a FunctionRegistry class holding them all. Note that this proposal differs from the dart-sass architecture on purpose to be more friendly to the way PHP works. This class is designed to rely on a big static array that can stay in OPCache shared memory rather than relying on lots of objects that need to be instantiated each time we create a compiler.
namespace ScssPhp\ScssPhp\Functions;
class FunctionRegistry
{
private static $functions = [
'str-index' => [ 'arguments' => [ 'string', 'subString' ], 'callback' => [ StringFunctions::class, 'index' ] ],
'rgb' => [ 'overloads' => [
[ 'arguments' => [ 'channels' ], 'callback' => [ ColorFunctions::class, 'rgbChannels' ] ],
[ 'arguments' => [ 'color' , 'alpha'], 'callback' => [ ColorFunctions::class, 'rgbTwoArgs' ] ],
[ 'arguments' => [ 'red', 'green', 'blue'], 'callback' => [ ColorFunctions::class, 'rgb' ] ],
[ 'arguments' => [ 'red', 'green', 'blue', 'alpha'], 'callback' => [ ColorFunctions::class, 'rgb' ] ],
] ],
'scale-color' => [ 'arguments' => [ 'color' ], 'restArgument' => 'kwargs', 'callback' => [ ColorFunctions::class, 'scale' ] ],
// Lots of other functions
];
/**
* @param string $name The normalized function name (i.e. using dashes, not underscores)
*
* @return array|null To be confirmed
*/
public static getFunction($name)
{
// ...
}
}The exact type of the return from getFunction remains to be defined. It could either be array|null with the array being what we have in the registry, or it could be some object wrapping it. I might keep the array for performance reasons as that's purely internal.
The exact structure of the array for arguments remains to be defined, in order to deal with default values in the most efficient way.
restArgument is separate from other arguments as its processing is different. The data structure is meant to represent all the necessary info without needing extra parsing at runtime.
Custom functions registered in the compiler will use a similar storage but in a private property of the Compiler, as they must not be shared between compiler instances.
The API for Composer::registerFunction still need to be discussed, to see whether we expose the internal format or whether we expect an argument declaration string that gets parsed (or something else)
Requirements
This proposal depends on #265 so that the implementation of functions would not need to depend on Compiler APIs like $this->reduce() and $this->compileValue anymore (usages of $this->error() will be replaced with throwing SassScriptException, which is meant for that).
This proposal also requires that extending the compiler does not allow overriding or calling lib* methods in a way where removing them triggers a BC break in a minor version. This implies one of these 3 requirements:
- the refactoring is done in the 2.0 release (where BC breaks are OK) => this forces to delay 2.0 until this refactoring is done, which does not help an incremental rewrite
- 2.0 forbids extending the Compiler entirely (making it
final) - 2.0 forbids overriding and calling
lib*functions when extending the compiler (could be done by changing all these methods toprivatein 2.0)
Implementation plan
Once the requirements are met, we can introduce the new registry in parallel of the magic discovery (preferring the new registry by checking it first), migrate functions module by module and then remove the magic discovery (if we don't make the compiler itself final in 2.0, we can actually only deprecate the magic discovery but not remove it, due to the child classes potentially using it).
TODOs
- Define the representation of arguments in the registry to support default values (which must be represented using scalars or arrays of scalars, not
Value, to keep the big registry array a static array) - Decide on the return value of
FunctionRegistry::getFunction - Decide on the API for
Compiler::registerFunction