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

feat: STRF-9749 Update Stencil Cli to use node-sass latest by default #923

Merged
merged 1 commit into from
Apr 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/pull_request_review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ jobs:

steps:
- name: Checkout code
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: Use Node.js ${{ matrix.node }}
uses: actions/setup-node@v2-beta
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node }}

Expand Down
7 changes: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
### Draft

- feat: strf-9749 Update Stencil Cli to use node-sass latest by default ([923](https:/bigcommerce/stencil-cli/pull/923))
New option:` --use-old-node-sass-fork` to bypass error warning (in case it exist) using latest node-sass version.
Soon, it node sass fork will be deprecated and it won't be possible to compile with it.
jairo-bc marked this conversation as resolved.
Show resolved Hide resolved

- feat: STRF-9757 Introduced "stencil debug" ([918](https:/bigcommerce/stencil-cli/pull/918))

Available options: --output [filename] (-o)
If not provided, will be logged to std
- feat: STRF-9741 Verbose network requests logging in Stencil CLI by default ([x](https:/bigcommerce/stencil-cli/pull/x))

- feat: STRF-9741 Verbose network requests logging in Stencil CLI by default ([914](https:/bigcommerce/stencil-cli/pull/914))
Introduced `--no-verbose` option on all commands to supress verbose network requests logging.

### 4.0.0 (2022-04-11)
Expand Down
10 changes: 10 additions & 0 deletions lib/StencilCLISettings.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ class StencilCLISettings {
constructor() {
// The setting enables/disables verbose network requests logging
this.versboseNetworkLogging = true;
// Enables usage of node sass fork for testing purposes
this.oldNodeSassFork = false;
}

setVerbose(flag) {
Expand All @@ -11,6 +13,14 @@ class StencilCLISettings {
isVerbose() {
return this.versboseNetworkLogging === true;
}

useOldNodeSassFork(flag) {
this.oldNodeSassFork = flag;
}

isOldNodeSassForkEnabled() {
return this.oldNodeSassFork;
}
}

const settings = new StencilCLISettings();
Expand Down
96 changes: 84 additions & 12 deletions lib/bundle-validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const privateThemeConfigValidationSchema = require('./schemas/privateThemeConfig
const themeConfigValidationSchema = require('./schemas/themeConfig.json');
const themeValidationSchema = require('./schemas/themeSchema.json');
const cssCompiler = require('./css/compile');
const stencilCLISettings = require('./StencilCLISettings');

const VALID_IMAGE_TYPES = ['.jpg', '.jpeg', '.png', '.gif'];
const WIDTH_COMPOSED = 600;
Expand All @@ -27,6 +28,8 @@ const MAX_SIZE_COMPOSED = 1024 * 1024 * 2; // 2MB
const MAX_SIZE_MOBILE = 1024 * 1024; // 1MB
const MAX_SIZE_DESKTOP = 1024 * 1024 * 5; // 5MB

/* eslint-disable no-useless-escape */
const STYLESHEET_REGEXP = /{{\s*stylesheet\s*([\/a-zA-Z'"\.-]+)\s*}}/i;
class BundleValidator {
/**
* Run some validations to ensure that the platform will accept the theme
Expand Down Expand Up @@ -408,31 +411,100 @@ class BundleValidator {
const assetsPath = path.join(this.themePath, 'assets');
const stylesPath = path.join(this.themePath, 'assets/scss');
const rawConfig = await this.themeConfig.getRawConfig();
const compiler = rawConfig.css_compiler;

let cssFiles;
try {
const files = await fs.promises.readdir(stylesPath);
cssFiles = files.filter((file) => {
return file.slice(-(compiler.length + 1)) === `.${compiler}`;
});
} catch (e) {
throw new Error(`Error: ${e.message}, while reading files from "${stylesPath}".`.red);
}
const cssFiles = await this.getCssFiles();

for (const file of cssFiles) {
try {
const engine = stencilCLISettings.isOldNodeSassForkEnabled()
? cssCompiler.FALLBACK_SASS_ENGINE_NAME
: cssCompiler.SASS_ENGINE_NAME;
/* eslint-disable-next-line no-await-in-loop */
await cssCompiler.compile(rawConfig, assetsPath, file);
await cssCompiler.compile(rawConfig, assetsPath, file, engine);
} catch (e) {
const message = this.parseStencilStylesError(e);
if (!stencilCLISettings.isOldNodeSassForkEnabled()) {
throw new Error(
`${message}\n`.red +
`\n` +
`---------WARNING---------\n`.red +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you okay with such wordings or do we need to adjust?

cc @bookernath

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good 👍 is there some link we can provide to help people diagnose errors? Some node-sass documentation perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are bunch of links, that you need to really dig to understand the context:
https:/sass/node-sass/releases/tag/v3.5.1 - the release, that broke and changelogs.
https:/bigcommerce/cornerstone/pull/999/files#diff-7b10e2a8164a3ef7ccb5aa035f393c9715b8945fcc59613d455408c505dd42a5L4 - known example of broken theme case (cornernstone)
sass/libsass#1550 - code examples
sass/sass#779 - code examples

@bookernath Maybe we can include all of them into out changelog and show the link for the changelog?

`We are currently in the process of deprecating node-sass fork https:/bigcommerce-labs/node-sass\n` +
`Your scss files were compiled using latest node-sass version https:/sass/node-sass\n` +
`This error might indicate that your scss file is not compatible with it.\n` +
`There is still an option to compile scss file old fork by using --use-old-node-sass-fork.\n` +
`But note, that this will lead to 500 error in production in near future.\n` +
jairo-bc marked this conversation as resolved.
Show resolved Hide resolved
`---------WARNING---------\n`.red,
);
}
throw new Error(
`${message} while compiling css files from "${stylesPath}/${file}".`.red,
);
}
}
}

async getCssFiles() {
const templatesPath = path.join(this.themePath, 'templates');
const files = await recursiveReadDir(templatesPath);
const cssFiles = [];
for await (const file of files) {
const content = await fs.promises.readFile(file, { encoding: 'utf-8' });
const result = content.match(STYLESHEET_REGEXP);
if (result) {
// remove quotes
const fileName = result[1].slice(1, -1);

const filePath = this.tryToResolveCssFileLocation(fileName, result);
if (!cssFiles.includes(filePath)) {
// check if file exist
cssFiles.push(filePath);
}
}
}

return cssFiles;
}

// returns relative path starting from root scss/css folder
tryToResolveCssFileLocation(fileName, result) {
const possibleLocations = [
fileName,
fileName.replace('/css/', '/scss/'),
fileName.replace('/scss/', '/css/'),
fileName.replace('/css/', '/scss/').replace('.css', '.scss'),
fileName.replace('/scss/', '/css/').replace('.scss', '.css'),
];

for (const location of possibleLocations) {
const filePath = path.join(this.themePath, location);
if (fs.existsSync(filePath)) {
if (!this.isRootCssFile(location)) {
return this.getCssFileWithoutRootFolder(location);
}
const fileParts = path.parse(filePath);
return fileParts.name;
}
}

throw new Error(`Couldn't find file for this statement: ${result[0]}`.red);
}

// root folders are /assets/css /assets/scss
// so after split, there can be 3 or 4 elements in the array (depending if the leading slash is present)
isRootCssFile(location) {
return location.split('/').length <= 4;
}

getCssFileWithoutRootFolder(location) {
const locationParts = location.split('/');
if (locationParts[0] === '') {
locationParts.shift();
}
locationParts.shift();
locationParts.shift();

return locationParts.join('/');
}

parseStencilStylesError(e) {
if (e.formatted) {
return `${e.formatted.replace('Error: ', '')}: `;
Expand Down
16 changes: 16 additions & 0 deletions lib/bundle-validator.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const { promisify } = require('util');

const ThemeConfig = require('./theme-config');
const BundleValidator = require('./bundle-validator');
const stencilCLISettings = require('./StencilCLISettings');

const themePath = path.join(process.cwd(), 'test/_mocks/themes/valid');

Expand All @@ -15,6 +16,7 @@ describe('BundleValidator', () => {

afterEach(() => {
jest.restoreAllMocks();
stencilCLISettings.useOldNodeSassFork(false);
});

it('should not run image validations for private themes', async () => {
Expand Down Expand Up @@ -126,4 +128,18 @@ describe('BundleValidator', () => {
'while parsing frontmatter',
);
});

it('should throw an error using latest node sass', async () => {
const themePath2 = path.join(
process.cwd(),
'test/_mocks/themes/invalid-scss-latest-node-sass',
);
themeConfig = ThemeConfig.getInstance(themePath2);

const validator = new BundleValidator(themePath2, themeConfig, false);

await expect(promisify(validator.validateTheme.bind(validator))()).rejects.toThrow(
'Import directives may not be used within control directives or mixins',
);
});
});
32 changes: 32 additions & 0 deletions lib/bundle-validator2.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
const path = require('path');
const { promisify } = require('util');

const ThemeConfig = require('./theme-config');
const BundleValidator = require('./bundle-validator');
const stencilCLISettings = require('./StencilCLISettings');

// NOTE:
// Since stencil styles can't change node sass engine in runtime, tests are divided into 2 files
// Jest runs tests in parallel, so the node sass binaries are not conflicting and we are able to test both scenarios
//
// Skipping this test until multiple core are supported no Github Actions
describe.skip('BundleValidator', () => {
afterEach(() => {
jest.restoreAllMocks();
stencilCLISettings.useOldNodeSassFork(false);
});

it('should compile successfully with old node sass fork', async () => {
const themePath = path.join(
process.cwd(),
'test/_mocks/themes/invalid-scss-latest-node-sass',
);
const themeConfig = ThemeConfig.getInstance(themePath);
stencilCLISettings.useOldNodeSassFork(true);

const validator = new BundleValidator(themePath, themeConfig, false);

const result = await promisify(validator.validateTheme.bind(validator))();
expect(result).not.toBeNull();
});
});
6 changes: 6 additions & 0 deletions lib/cliCommon.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,17 @@ function applyCommonOptions(program) {
program
.option('-h, --host [hostname]', 'specify the api host')
.option('-nov, --no-verbose', 'supress verbose info logging', false)
.option(
'--use-old-node-sass-fork',
jairo-bc marked this conversation as resolved.
Show resolved Hide resolved
'use old node sass fork for scss compilation during bundling',
false,
)
.parse(process.argv);
}

function setupCLI(options) {
stencilCLISettings.setVerbose(options.verbose);
stencilCLISettings.useOldNodeSassFork(options.useOldNodeSassFork);
}

function prepareCommand(program) {
Expand Down
9 changes: 6 additions & 3 deletions lib/css/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ const { promisify } = require('util');

const cssAssembler = require('../css-assembler');

const SASS_ENGINE_NAME = 'node-sass-fork';
const FALLBACK_SASS_ENGINE_NAME = 'node-sass-fork';
const SASS_ENGINE_NAME = 'node-sass';

const compile = async (configuration, themeAssetsPath, fileName) => {
const compile = async (configuration, themeAssetsPath, fileName, engineName = SASS_ENGINE_NAME) => {
const fileParts = path.parse(fileName);
const ext = configuration.css_compiler === 'css' ? configuration.css_compiler : 'scss';
const pathToFile = path.join(fileParts.dir, `${fileParts.name}.${ext}`);
Expand All @@ -32,11 +33,13 @@ const compile = async (configuration, themeAssetsPath, fileName) => {
},
};
const stencilStyles = new StencilStyles(console);
stencilStyles.activateEngine(SASS_ENGINE_NAME);
stencilStyles.activateEngine(engineName);

return stencilStyles.compileCss('scss', params);
};

module.exports = {
compile,
SASS_ENGINE_NAME,
FALLBACK_SASS_ENGINE_NAME,
};
4 changes: 4 additions & 0 deletions lib/stencil-push.utils.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ describe('stencil push utils', () => {
channelIds: [1, 2],
};

beforeEach(() => {
jest.spyOn(console, 'info').mockImplementation(jest.fn());
});

afterEach(() => {
jest.restoreAllMocks();
axiosMock.reset();
Expand Down
1 change: 1 addition & 0 deletions server/plugins/renderer/renderer.module.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ describe('Renderer Plugin', () => {
// Prevent littering the console
jest.spyOn(console, 'log').mockImplementation(jest.fn());
jest.spyOn(console, 'error').mockImplementation(jest.fn());
jest.spyOn(console, 'info').mockImplementation(jest.fn());

server = await Server.create(serverOptions);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
h1 {
color: #000000;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
h1 {
color: #0000AA;
}

$font: "Arial";

@if not contains($font, "'Clear Sans', sans-serif") {
@import "test";
}
Loading