(feat): add missing @babel/preset-react dependency and add @Sage alia#44
Conversation
| const args = [ | ||
| ...( glob?.path ? [ glob.path ] : [] ), | ||
| ...( userPath ? [ userPath ] : [] ), | ||
| '--check', | ||
| '--write', | ||
| ]; | ||
|
|
||
| const child = spawn( command, args, { | ||
| stdio: 'inherit', | ||
| shell: true, // required for glob support like {*,**/*}.css | ||
| } ); | ||
|
|
||
| child.on( 'exit', ( code ) => { | ||
| if ( code !== 0 ) { | ||
| error( `Prettier exited with code ${ code }` ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
Ik ga dit nog herschrijven, want dubbele code met lint.js, komt in de andere PR nadat deze gemerged is
| { | ||
| filetype: filetypes.css.name, | ||
| path: './web/app/themes/**/resources/styles/**/*.css', | ||
| path: './web/app/themes/**/resources/styles/{*,**/*}.css', |
There was a problem hiding this comment.
Met path: './web/app/themes/**/resources/styles/{*,**/*}.css', pakte hij ./web/app/themes/**/resources/styles/frontend.css niet. Met deze glob nu wel.
| const command = 'prettier'; | ||
|
|
||
| const glob = getGlobByFormatModeAndFiletype( | ||
| formatMode, | ||
| formatFiletype.name | ||
| ); | ||
|
|
||
| run( `${ command } ${ glob?.path ?? '' } ${ userPath ?? '' }`, 'format' ); | ||
| const args = [ | ||
| ...( glob?.path ? [ glob.path ] : [] ), | ||
| ...( userPath ? [ userPath ] : [] ), | ||
| '--check', | ||
| '--write', | ||
| ]; |
There was a problem hiding this comment.
Dit was trouwens het probleem waar we eerder tegenaan liepen. Met run( ${ command } ${ glob?.path ?? '' } ${ userPath ?? '' }, 'format' ); werden de aanhalingstekens niet meegenomen:
prettier ./web/app/themes/**/resources/styles/{*,**/*}.css --write
Terwijl we die wel nodig hadden voor het command
prettier './web/app/themes/**/resources/styles/{*,**/*}.css' --write
Met spawn() en het gebruik van een args array worden de argumenten nu wel goed doorgegeven.
There was a problem hiding this comment.
Interessant, glob?.path is toch gewoon een string. Dus '${ glob?.path ?? '' }' had denk ik ook kunnen werken.
Overigens deze manier (met een args array) lijkt mij veel netter!
Wellicht kan deze logica nog weg geabstraheerd worden naar een helper of wellicht in run (of niet :) )
There was a problem hiding this comment.
Nope, '${ glob?.path ?? '' }' met aanhalingstekens werkt ook niet - de aanhalingstekens werden gestript
YvetteNikolov
requested review from
a team,
WybeBosch,
Yannicvanveen,
hnccox-yard,
laravdiemen and
rivanuff
and removed request for
a team
|
Eyo is babel nogsteeds nodig? Dat was toch voor like transpiling js naar oudere js zodat het cross browser compatible blijft? |
|
|
|
||
| settings: { | ||
| 'import/resolver': { | ||
| alias: [ |
There was a problem hiding this comment.
Dit is neem ik aan zodat hij Vite begrijpt? Ik neem aan dat het geen probleem is als deze config in een project met webpack wordt gebruikt, tock?
There was a problem hiding this comment.
Ook in oude projecten gaat dit fout en gaat de linter kapot
Dit is bijv. hier https://github.com/yardinternet/brave-scaffold/blob/main/stubs/sage-child/resources/scripts/frontend/frontend.js
| "chalk": "^5.4.1", | ||
| "eslint": "^9.32.0", | ||
| "meow": "^13.2.0", | ||
| "prettier": "npm:wp-prettier@^3.0.3", |
There was a problem hiding this comment.
Was dit dus toch nodig voor de override? Of is dit voor de verbositeit?
There was a problem hiding this comment.
Nu nodig omdat we prettier direct gebruiken in het format.js bestand
There was a problem hiding this comment.
Nu je het zegt, kunnen we de overrides weghalen. Ga ik doen.
| "stylelint": "^16.23.0" | ||
| }, | ||
| "optionalDependencies": { | ||
| "@yardinternet/eslint-config": "*", |
There was a problem hiding this comment.
Is any version wel handig? Zo meteen ontstaan er in de toekomst compatibiliteit problemen in oude projecten (een oude versie an de Toolkit CLI maar een nieuwe config). Anderzijds is het wel handig dat het dan automatisch 'geüpdatet' zonder dat je een nieuwe versie moet uitbrengen.
There was a problem hiding this comment.
Ja dat laatste. Als de toolkit geupdatet wordt in een project, doen we dat vaak via zo'n command die alle @yardinternet/ dependencies updatet.
Het is dan apart dat hier bij optionalDependencies een mogelijk oude versie staat.
| '--write', | ||
| ]; | ||
|
|
||
| const child = spawn( command, args, { |
There was a problem hiding this comment.
Is het niet beter om de exec in de run helper gewoon te vervangen voor de deze spawn functie? De functionaliteit is toch het zelfde? Alleen de logs worden wel getoond
There was a problem hiding this comment.
Laat maar ik zie nu je opmerking hierover: https://github.com/yardinternet/toolkit/pull/44/files#r2239860906
| const command = 'prettier'; | ||
|
|
||
| const glob = getGlobByFormatModeAndFiletype( | ||
| formatMode, | ||
| formatFiletype.name | ||
| ); | ||
|
|
||
| run( `${ command } ${ glob?.path ?? '' } ${ userPath ?? '' }`, 'format' ); | ||
| const args = [ | ||
| ...( glob?.path ? [ glob.path ] : [] ), | ||
| ...( userPath ? [ userPath ] : [] ), | ||
| '--check', | ||
| '--write', | ||
| ]; |
There was a problem hiding this comment.
Interessant, glob?.path is toch gewoon een string. Dus '${ glob?.path ?? '' }' had denk ik ook kunnen werken.
Overigens deze manier (met een args array) lijkt mij veel netter!
Wellicht kan deze logica nog weg geabstraheerd worden naar een helper of wellicht in run (of niet :) )
|
|
||
| const child = spawn( command, args, { | ||
| stdio: 'inherit', | ||
| shell: true, // Required for glob support |
There was a problem hiding this comment.
interessant dat je daar een optie voor moet mee geven. Ik zal denken dat dat gewoon binnen de 'stylelint' code wordt afgehandeld
There was a problem hiding this comment.
https://nodejs.org/api/child_process.html#child_processspawncommand-args-options
spawn() gebruikt standaard geen shell en in de shell worden de glob patterns geïnterpreteerd. Als ik dit niet zou doorgeven, zal de glob letterlijk doorgegeven worden als string.
3 participants
…s resolver
@sage/aliasses kunnen gebruiken@wordpress/scriptsvervangen metprettierenstylelintDoei 657 packages 👋