Replace `node-sass` with `sass-embedded` #1001

AMoo-Miki · 2023-08-29T20:00:23Z

Description

Replace node-sass with sass-embedded

Also:

Implement sass variable extraction
Implement sass variable importing in typescript
Add guards to post-install cleanup
Make sources compatible with dart sass by wrapping divisions in calc() and adding units were necessary
Fold in @elastic/charts/dist/theme.scss to modify it for dart-sass compatibility
Break out compiling charts

closes #966

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

BSFishy

initial review, still need to look at variable from scss when my brain isnt completely fried

scripts/compile-themes.js

src/components/breadcrumbs/_breadcrumbs.scss

src/components/form/range/_range_highlight.scss

src/components/form/range/_range_slider.scss

BSFishy · 2023-08-29T21:12:55Z

src/components/form/range/_range_thumb.scss

@@ -16,7 +16,7 @@
  position: absolute;
  left: 0;
  top: 50%;
-  margin-top: -($ouiRangeThumbHeight / 2);
+  margin-top: calc(-1 * $ouiRangeThumbHeight / 2);


Does this need to be -1 * x? What about $ouiRangeThumbHeight / -2?

BSFishy · 2023-08-29T21:30:06Z

src/components/tool_tip/_tool_tip.scss

+  $arrowPlusSize: (calc($arrowSize/2) + 1px) * -1; /* 1 */
+  $arrowMinusSize: (calc($arrowSize/2) - 1px) * -1; /* 1 */


I feel like these equations could be optimized but that could be done in a followup

src/themes/charts/themes.ts

src-docs/src/theme_dark.scss

joshuarrrr

A couple questions, but looks good to me. The important part is that it works! 😁

.gitignore

package.json

src-docs/src/views/header/header_dark.tsx

src-docs/src/views/utility_classes/utility_classes_responsive.js

src/components/call_out/_mixins.scss

SergeyMyssak

Good job! I added a couple of minor comments on code style but they can be ignored.

I was wondering if you used prettier in the new files, I have Webstorm showing that the file can be formatted

SergeyMyssak · 2023-09-02T02:19:56Z

scripts/babel/variables-from-scss/index.js

+const idKey = '!!variables-from-scss!!';
+const keyLength = idKey.length;
+
+const forbiddenKeyNames = [...Object.getOwnPropertyNames(Object.prototype), 'prototype'];


We can write them in uppercase to note that these variables are constant and should not be changed:

const IMPORT_PREFIX = '!!variables-from-scss!!'; const FORBIDDEN_KEYS = [...Object.getOwnPropertyNames(Object.prototype), 'prototype'];

I reserve uppercased names for constants that get exported; here they are just local variables. Do you have a strong feeling for this? or do you think they should be exported?

SergeyMyssak · 2023-09-02T02:48:37Z

scripts/babel/variables-from-scss/index.js

+        const { variables } = compileWithVariablesSync(importTarget);
+
+        // If no default specifier is used, reduce the variables to only those needed
+        const importedVariables = assignments.length === 0 ? usedVariableNames.reduce((acc, name) => {
+          if (!forbiddenKeyNames.includes(name)) acc[name] = variables[name];
+          return acc;
+        }, {}) : variables;


We can put the logic in a separate function to improve readability:

const filterImportedVariables = (allVariables, usedVariableNames) => { return usedVariableNames.reduce((acc, name) => { if (!FORBIDDEN_KEYS.includes(name)) { acc[name] = allVariables[name]; } return acc; }, {}); };

const importTarget = join(dirname(state.file.opts.filename), path.node.source.value.substring(IMPORT_PREFIX.length)); const { variables: allVariables } = compileWithVariablesSync(importTarget); const importedVariables = assignments.length === 0 ? filterImportedVariables(allVariables, usedVariableNames) : allVariables;

I have a feeling that these are called synchronously and recursively and was very worried about stack overflows; hence I inlined everything without looking at the depth.

SergeyMyssak · 2023-09-02T03:14:30Z

scripts/lib/compile-scss-with-variables.js

+const varListMarker = ':֍෴֍߷';
+const arrayMarker = '෴';
+const objectMarker = '߷';
+const forbiddenKeyNames = [...Object.getOwnPropertyNames(Object.prototype), 'prototype'];


We can write them in uppercase to note that these variables are constant and should not be changed:

const VAR_LIST_MARKER = ':֍෴֍߷'; const ARRAY_MARKER = '෴'; const OBJECT_MARKER = '߷'; const FORBIDDEN_KEY_NAMES = [...Object.getOwnPropertyNames(Object.prototype), 'prototype'];

Since they are local variables and not exported, I didn't; do you have a strong feeling towards this?
PS, for the same reason, i didn't deepFreeze the forbidden key names.

SergeyMyssak · 2023-09-02T03:17:39Z

scripts/lib/compile-scss-with-variables.js

+  // originalKey is only used for displaying errors
+  const _set = (o, path, value, originalKey) => {
+    // To improve performance, since this is a private method, it is not validating `path`
+    const [key, type, ...rest] = path;
+    if (forbiddenKeyNames.includes(key)) return;
+    if (rest.length === 0) {
+      if (type) {
+        throw new Error(`Unexpected type ${type} for ${key} was found on ${originalKey}`);
+      }
+      // For bwc, 0px -> 0
+      o[key] = isFinite(value) ? parseFloat(value) : (value === '0px' ? 0 : value);
+    } else {
+      switch (type) {
+        case arrayMarker:
+          if (key in o) {
+            if (!Array.isArray(o[key]))
+              throw new Error(`${key} of ${originalKey} was found to be ${Object.prototype.toString.call(o[key])}`);
+          } else {
+            o[key] = [];
+            // Inserting to the beginning to make sure the inner references appear before the outer ones.
+            foundArrays.unshift({ node: o, key });
+          }
+          _set(o[key], rest, value);
+          break;
+
+        case objectMarker:
+          if (key in o) {
+            if (Object.prototype.toString.call(o[key]) !== '[object Object]')
+              throw new Error(`${key} of ${originalKey} was found to be ${Object.prototype.toString.call(o[key])}`);
+          } else {
+            o[key] = {};
+            knownKeys.add(key);
+          }
+          _set(o[key], rest, value);
+          break;
+
+        default:
+          throw new Error(`Unknown type [${type}] in ${originalKey}`);
+      }
+    }
+  };


We can move this function somewhere else to avoid overloading this part of the code

We should; let's hold of though until OUI 2 when we remove all of those backward compatibility stuff or else we will have to pass foundArrays and knownKeys around.

joshuarrrr · 2023-09-05T23:35:33Z

This will follow the 1.3.0 release, as it only affects dev dependencies.

BSFishy · 2023-09-18T17:43:15Z

scripts/compile-themes.js

+  compileScssFiles(path.join('src', 'theme_*.scss'), 'dist', ouiPackageName)
+      .catch((err) => {
+        console.error(err);
+        process.exit(2);
+      });


Thinking about it again, this function shouldn't ever throw an error, I think. Everything that might is wrapped in a try-catch

There is a re-throw in compileScssFiles.

Also: * Implement sass variable extraction * Implement sass variable importing in typescript * Add guards to post-install cleanup * Make sources compatible with dart sass by wrapping divisions in `calc()` and adding units were necessary * Fold in `@elastic/charts/dist/theme.scss` to modify it for dart-sass compatibility * Break out compiling `charts` Signed-off-by: Miki <[email protected]>

Also: * Implement sass variable extraction * Implement sass variable importing in typescript * Add guards to post-install cleanup * Make sources compatible with dart sass by wrapping divisions in `calc()` and adding units were necessary * Fold in `@elastic/charts/dist/theme.scss` to modify it for dart-sass compatibility * Break out compiling `charts` Signed-off-by: Miki <[email protected]> (cherry picked from commit db94845) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Also: * Implement sass variable extraction * Implement sass variable importing in typescript * Add guards to post-install cleanup * Make sources compatible with dart sass by wrapping divisions in `calc()` and adding units were necessary * Fold in `@elastic/charts/dist/theme.scss` to modify it for dart-sass compatibility * Break out compiling `charts` (cherry picked from commit db94845) Signed-off-by: Miki <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Also: * Implement sass variable extraction * Implement sass variable importing in typescript * Add guards to post-install cleanup * Make sources compatible with dart sass by wrapping divisions in `calc()` and adding units were necessary * Fold in `@elastic/charts/dist/theme.scss` to modify it for dart-sass compatibility * Break out compiling `charts` Signed-off-by: Miki <[email protected]> Signed-off-by: ShatilKhan <[email protected]>

AMoo-Miki requested review from seanneumann, rednaksi91, KrooshalUX, BSFishy, bandinib-amzn, ashwin-pc, joshuarrrr, andreymyssak and SergeyMyssak as code owners August 29, 2023 20:00

github-actions bot added the all-star-contributor label Aug 29, 2023

AMoo-Miki force-pushed the dart-experiment branch 3 times, most recently from 9456f45 to 7dcbe48 Compare August 29, 2023 20:09

AMoo-Miki added the backport 1.x label Aug 29, 2023

AMoo-Miki force-pushed the dart-experiment branch 2 times, most recently from 7072912 to 7f81d1b Compare August 30, 2023 18:13

BSFishy reviewed Aug 30, 2023

View reviewed changes

AMoo-Miki force-pushed the dart-experiment branch from 7f81d1b to a6e4271 Compare August 30, 2023 19:53

joshuarrrr reviewed Aug 30, 2023

View reviewed changes

joshuarrrr approved these changes Aug 30, 2023

View reviewed changes

joshuarrrr added the 1.3 label Aug 30, 2023

SergeyMyssak approved these changes Sep 2, 2023

View reviewed changes

andreymyssak approved these changes Sep 2, 2023

View reviewed changes

joshuarrrr removed the 1.3 label Sep 5, 2023

BSFishy mentioned this pull request Sep 14, 2023

Replace sass-lint with stylelint #949

Open

7 tasks

BSFishy reviewed Sep 18, 2023

View reviewed changes

AMoo-Miki force-pushed the dart-experiment branch from a6e4271 to b7b5dc6 Compare September 26, 2023 21:25

AMoo-Miki merged commit db94845 into opensearch-project:main Sep 27, 2023

opensearch-trigger-bot bot mentioned this pull request Sep 27, 2023

[Backport 1.x] Replace node-sass with sass-embedded #1054

Merged

AMoo-Miki deleted the dart-experiment branch September 27, 2023 21:11

BSFishy mentioned this pull request Sep 29, 2023

Migrate to Dart Sass #942

Closed

7 tasks

This was referenced Oct 10, 2023

Use dart-sass instead of node-sass #160

Closed

[Research] Research the usage of package node-sass in OUI #633

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `node-sass` with `sass-embedded` #1001

Replace `node-sass` with `sass-embedded` #1001

AMoo-Miki commented Aug 29, 2023 •

edited

Loading

BSFishy left a comment

BSFishy Aug 29, 2023

BSFishy Aug 29, 2023

joshuarrrr left a comment

SergeyMyssak left a comment

SergeyMyssak Sep 2, 2023

AMoo-Miki Sep 6, 2023

SergeyMyssak Sep 2, 2023

AMoo-Miki Sep 6, 2023

SergeyMyssak Sep 2, 2023

AMoo-Miki Sep 6, 2023

SergeyMyssak Sep 2, 2023

AMoo-Miki Sep 6, 2023

joshuarrrr commented Sep 5, 2023

BSFishy Sep 18, 2023

AMoo-Miki Sep 26, 2023

		$arrowPlusSize: (calc($arrowSize/2) + 1px) * -1; /* 1 */
		$arrowMinusSize: (calc($arrowSize/2) - 1px) * -1; /* 1 */

Replace node-sass with sass-embedded #1001

Replace node-sass with sass-embedded #1001

Conversation

AMoo-Miki commented Aug 29, 2023 • edited Loading

Description

Check List

BSFishy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarrrr left a comment

Choose a reason for hiding this comment

SergeyMyssak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuarrrr commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Replace `node-sass` with `sass-embedded` #1001

Replace `node-sass` with `sass-embedded` #1001

AMoo-Miki commented Aug 29, 2023 •

edited

Loading