-
Notifications
You must be signed in to change notification settings - Fork 238
Update components compilation to use pnpm #1413
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1413 +/- ##
==========================================
Coverage 86.51% 86.51%
==========================================
Files 340 340
Lines 85987 85992 +5
Branches 4825 3184 -1641
==========================================
+ Hits 74392 74397 +5
- Misses 11572 11595 +23
+ Partials 23 0 -23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dpvc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, both these changes are problematic.
The shell setting is required in Windows. I guess one could use
shell: process.platform === 'Win32'As for npx vs pnpm, these don't seem to handle locating the needed webpack command the same way, and while pnpm works within the MathJax-src repository to build MathJax itself, the pack command is used for building other things, like the fonts or third-party extensions or custom MathJax builds, and it doesn't seem to work for those. I remember trying to figure that out some time ago, and not being able to get it to work with pnpm. I can try again, if you think it is necessary, but npx works, so I kept it.
That would at least help with the annoying security complaints in between the webpack output.
The main reason for replacing this was that I got annoying output along the line of: and I could not find any of these in my local configuration.Do you get similar output or, if it is just me, it might be that I have neglected my |
This output apparently was introduced in The difficulty with switching Here is one possible solution: diff --git a/components/bin/pack b/components/bin/pack
index 4a91ab5de..22236ed7d 100755
--- a/components/bin/pack
+++ b/components/bin/pack
@@ -74,9 +74,9 @@ async function readJSON(dir) {
return new Promise((ok, fail) => {
const buffer = [];
const child = spawn(
- 'npx',
+ 'node',
[
- 'webpack',
+ require.resolve(path.join('webpack', 'bin', 'webpack.js')),
'--env', `dir=${path.relative(packDir, path.resolve(dir))}`,
'--env', `bundle=${bundle}`,
'--json',
@@ -84,7 +84,7 @@ async function readJSON(dir) {
],
{
cwd: packDir,
- shell: true,
+ shell: process.platform === 'Win32',
}
);
child.stdout.on('data', (data) => buffer.push(String(data)));This looks up the webpack executable by hand and uses Is that acceptable? |
dpvc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
PR updates the
packscript to usepnpminstead ofnpx.Secondly, I set the
shelloption to false as this was giving me security complains during web packing. I don't think we need it, as we do not do anything we need the shell for, like piping etc.