Conversation
| jestPreviewConfigure({ | ||
| externalCss: ['src/index.css', 'src/global-style.scss'], | ||
| sassLoadPaths: ['src'], // Root for @use, @import | ||
| }); |
There was a problem hiding this comment.
@ntt261298 Is there any reason we need to move jestPreviewConfigure from setupFilesAfterEnv to setupFiles?
There was a problem hiding this comment.
I thought that setupFilesAfterEnv will be run before the transformers, but actually both setupFilesAfterEnv and setupFiles are run before transformers, so I reverted to use setupFilesAfterEnv
| @@ -0,0 +1,7 @@ | |||
| @import './partial-style'; | |||
There was a problem hiding this comment.
I see the file is _partial-style.scss (more _). Is this Saas feature or a typo?
There was a problem hiding this comment.
- It's the feature of Sass,
_indicates that is a partial sass, so sass will not compile it to a CSS unless we use that in another sass file. - Also: .sass and .scss solve the same problem, but the syntax is a bit different, .scss has the syntax more similar to .css (contain semicolons, brackets) while .sass doesn't: https://www.geeksforgeeks.org/what-is-the-difference-between-scss-and-sass/#:~:text=SASS%20(Syntactically%20Awesome%20Style%20Sheets,of%20the%20existing%20CSS%20syntax.
src/configure.ts
Outdated
| if (cssFile.endsWith('.scss') || cssFile.endsWith('.sass')) { | ||
| const sass = require('sass'); | ||
|
|
There was a problem hiding this comment.
This is not safe since user might not have saas installed locally. Might need to try catch
There was a problem hiding this comment.
Yeah, also, sass-loader supports multiple sass implementation (node-sass, dart-sass,...), maybe we can refer to that
src/transform.ts
Outdated
| } catch { | ||
| // File doesn't exist | ||
| sassLoadPaths = JSON.stringify([]); | ||
| } |
There was a problem hiding this comment.
Can we guarantee the error is always File doesn't exist?
Can we assert if the error is exactly that case?
There was a problem hiding this comment.
I think we can move to use fs.existsSync to check of the config file exists
src/transform.ts
Outdated
| const sassSrc = ${JSON.stringify(src)}; | ||
|
|
||
| const result = sass.compileString(sassSrc, { | ||
| loadPaths: ${sassLoadPaths}, | ||
| }); |
There was a problem hiding this comment.
There was a problem hiding this comment.
We don't have other reason to convert sass to run time, so I move it out of the return string
src/configure.ts
Outdated
| // Transform sass to css | ||
| const sassDestinationFile = destinationFile.replace( | ||
| /\.(scss|sass)$/, | ||
| '.css', | ||
| ); |
There was a problem hiding this comment.
I think move this declaration next to fs.writeFileSync(sassDestinationFile, sassResult.css); is more readable
# Conflicts: # package-lock.json # package.json # src/configure.ts
… sass in transform.ts
src/configure.ts
Outdated
| ); | ||
| // Example: src/common/styles.css => cache-src___common___styles.css | ||
| const delimiter = '___'; | ||
| const destinationBasename = `cache-${basename.replace('/', delimiter)}`; |
There was a problem hiding this comment.
This will replace the first /.
| const destinationBasename = `cache-${basename.replace('/', delimiter)}`; | |
| const destinationBasename = `cache-${basename.replace(/\//g, delimiter)}`; |
There was a problem hiding this comment.
I don't think this logic correct since we are using basename
src/configure.ts
Outdated
|
|
||
| // If sass file is included, we need to transform it to css | ||
| if (cssFile.endsWith('.scss') || cssFile.endsWith('.sass')) { | ||
| const { exec } = require('child_process'); |
src/transform.ts
Outdated
| sass = require('sass'); | ||
| } catch (err) { | ||
| console.log(err); | ||
| return `module.exports = ${JSON.stringify(src)};`; |
There was a problem hiding this comment.
I think fileame is a better option for a fallback.
nvh95
left a comment
There was a problem hiding this comment.
- Can you update scss code to also use
@use? - Can you update scss code to use
~(e.g:@use '~nprogress/nprogress';) (loading a css file from the nprogress node module) - Reading Create React App's scss page gives me a lot of information (
@use, how they handleloadPathsvia environment variable SASS_PATH, node-sass and LibSass are deprecated, SCSS Modules) - Given
node-sass and LibSass are deprecated(need more source), let's consider if we need to support those processors. I think the strategy can be: release withDart Sass, then support other Sass processors in the next releases. If we go with that strategy, remember to mention we are currently support only Dart Sass. - Can you update scss code to
demo. That folder is to be used for development thanexamples/vite-react. I think we can add an other examplesassto showcase howjest-previewworks withsass(The idea is from Next.JS https://github.com/vercel/next.js/tree/canary/examples)
| exec( | ||
| `./node_modules/.bin/sass ${cssFile} ${cssDestinationFile} --no-source-map`, | ||
| (err: any) => { | ||
| if (err) { | ||
| console.log(err); | ||
| } | ||
| }, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Since the reason to use exec here is special (try not to run sass in jsdom environment), Can you add a detail comment here? @ntt261298?
|
|
||
| // Transform sass to css and save to cache folder | ||
| exec( | ||
| `./node_modules/.bin/sass ${cssFile} ${cssDestinationFile} --no-source-map`, |
There was a problem hiding this comment.
I have a little bad feeling about using ./node_modules/.bin/sass directly. I'm afraid it's not gonna work in some circumstances (e.g: monorepo? but we are not testing for monorepo just yet anyway).
I'm not sure, since this is pretty similar to the problem I have to solve when adding postinstall script.
No clear action items now. I think just to keep this implementation in mind for now.
| transform: { | ||
| '^.+\\.(ts|js|tsx|jsx)$': '@swc/jest', | ||
| '^.+\\.css$': 'jest-preview/transforms/css', | ||
| '^.+\\.(css|scss|sass)$': 'jest-preview/transforms/css', |
There was a problem hiding this comment.
This comment is to remind not forget to update README.md on CSS transform
| @@ -0,0 +1,5 @@ | |||
| header { | |||
| .global-configured-sass { | |||
| font-size: 40px; | |||
There was a problem hiding this comment.
I personally think we can use text-transform: uppercase; (or add a background color) would make us easy to realize if @import takes place yet. Feel free to leave your option.
Similar to _partial-style.scss
| @@ -0,0 +1,7 @@ | |||
| @import 'partials/partial-style'; | |||
There was a problem hiding this comment.
As I understand @use and @import are pretty the same, @use is newer with some advantages. So I think we can use both @use and @import in our example to cover more Sass features.
src/configure.ts
Outdated
| // Create cache folder if it doesn't exist | ||
| if (!fs.existsSync(CACHE_FOLDER)) { | ||
| fs.mkdirSync(CACHE_FOLDER, { | ||
| recursive: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I think we can add an utils.ts file which has createCacheFolderIfNeeded() or something like that. We have this logic in quite many places.
nvh95
left a comment
There was a problem hiding this comment.
Let's ship loadPaths and ~ in the next version.
# Conflicts: # examples/vite-react/package-lock.json
✅ Deploy Preview for jest-preview-library canceled.
|
Features
Todo:
loadPaths: Currently,jest-previewonly supports relative scss import via @import, @use. In real projects, we can add customloadPathsto let sass know where to find the imported files.~import: Example:@use '~nprogress/nprogress'. This is a feature ofsass-loader, or we can add additional config to achieve this. Example in Vite: