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

chore(postcss-zindex): refactor to drop dependencies #1096

Merged
merged 3 commits into from May 13, 2021
Merged

Conversation

ludofischer
Copy link
Collaborator

@ludofischer ludofischer commented May 12, 2021

Replace uniqs and has with a map.

Updated algorithm

  1. for each z-index, create a map from the z-index to itself
  2. for every map key, update the value to the optimized z-index
  3. replace every z-index by retrieving the value corresponding to that z-index key

Compared to the previous version, it has a few advantages

  1. no external dependencies
  2. fewer steps (no need to deduplicate an array)
  3. I find it clearer uses the same data structure from beginning to end, while previously it swapped an array with an object midway,

Replace uniqs and has with a map.
@@ -29,7 +29,7 @@ function pluginCreator(opts = {}) {
return;
}

cache.optimizeValues();
cache.optimizeValues(opts.startIndex || 1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The start index is only used inside optimizeValues() so I find passing it as an argument reduces indirection (instead of first storing it as an instance variable)

if (has(this._values, value)) {
return this._values[value];
LayerCache.prototype.optimizeValues = function (startIndex) {
const sortedValues = Array.from(this._values.keys()).sort(ascending);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Assign to each z-index its rank in the sorted array.

LayerCache.prototype.optimizeValues = function () {
this._values = uniq(this._values)
.sort(ascending)
.reduce(reduceValues.bind(this), {});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is where the old code does its 'magic'. this._values is initially an array, which then gets assigned to an object here.

return list;
}

LayerCache.prototype._findValue = function (value) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This whole method is not needed anymore, since we can access the map by key directly.

@codecov-commenter
Copy link

codecov-commenter commented May 12, 2021

Codecov Report

Merging #1096 (f35aba1) into master (4d7fe36) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1096      +/-   ##
==========================================
- Coverage   96.38%   96.37%   -0.01%     
==========================================
  Files         115      115              
  Lines        3595     3589       -6     
  Branches     1060     1059       -1     
==========================================
- Hits         3465     3459       -6     
  Misses        121      121              
  Partials        9        9              
Impacted Files Coverage Δ
packages/postcss-zindex/src/index.js 100.00% <100.00%> (ø)
packages/postcss-zindex/src/lib/layerCache.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d7fe36...f35aba1. Read the comment docs.

@ludofischer ludofischer changed the title chore(postcss-zindex): refactor to drop depenencies chore(postcss-zindex): refactor to drop dependencies May 12, 2021
@alexander-akait
Copy link
Member

Less deps === less problems

@ludofischer ludofischer merged commit 65d8197 into master May 13, 2021
@ludofischer ludofischer deleted the zindex-has branch May 13, 2021 17:01
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.

None yet

3 participants