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

Feature/uepr 40 desktop feature parity #461

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

KManolov3
Copy link

@KManolov3 KManolov3 commented Feb 10, 2025

Resolves

Resolves https://scratchfoundation.atlassian.net/browse/UEPR-140

Proposed Changes

  • Allow for test builds on Linux
  • Remove electron-webpack and handle webpack configuration manually

Reason for Changes

  • To be able to upgrade webpack to v5 - the version of GUI - and, consequently, catch up with latest GUI features

@KManolov3 KManolov3 marked this pull request as draft February 10, 2025 18:12
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

Wow, there's a lot going on here! There are a couple of things to change, but they're pretty minor... overall this looks good to me. It looks like maybe the development experience is even a little better than before...?

Do you think scratch-webpack-configuration would help here, or would that just complicate things? I'm not suggesting to add it to this PR, just curious about a possible future step.

package.json Outdated
Comment on lines 17 to 19
"build:dev": "NODE_ENV=production npm run compile && npm run doBuild -- --mode=dev",
"build:dir": "NODE_ENV=production npm run compile && npm run doBuild -- --mode=dir",
"build:dist": "NODE_ENV=production npm run compile && npm run doBuild -- --mode=dist",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is NODE_ENV=production necessary here?

Assuming it is necessary, could you set this up to use crossenv instead? That will allow Windows users to run these commands, too.

Copy link
Author

@KManolov3 KManolov3 Mar 26, 2025

Choose a reason for hiding this comment

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

This was previously set automatically via electron-webpack - the webpack config and the main process depend on it for setting the correct paths to load content from. Will update it with crossenv.

scripts/start.js Outdated
shell: true
});

mainProcess.on('exit', async (code) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... can't you do something like await spawn(...)?
(does some web searching)
...nevermind, this looks good. 😅

Comment on lines 23 to 24

const devToolKey = ((process.platform === 'darwin') ?
{ // macOS: command+option+i
const devToolKey = ((process.platform === 'darwin' || process.platform === 'linux') ?
{ // macOS / linux: command+option+i
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Linux wants the same control+shift+i key combo as Windows here

Choose a reason for hiding this comment

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

I think Linux wants the same control+shift+i key combo as Windows here

as a former linux user (and still somewhat uses it), i can confirm

const baseUrl = (isDevelopment ?
`http://localhost:${process.env.ELECTRON_WEBPACK_WDS_PORT}/` :
`file://${__dirname}/`
const baseUrl = (isDevelopment ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const baseUrl = (isDevelopment ?
const baseUrl = (isDevelopment ?

@KManolov3
Copy link
Author

Regarding scratch-webpack-configuration - I definitely see it as a natural further step. A lot of the webpack config already follows rules similar to scratch-gui / scratch-www where the unified config is used. Although given that this has already turned into a pretty large technical task that has a lot of potential breaking changes, it seems better to do this after we release the scratch-android / scratch-desktop changes

@KManolov3 KManolov3 marked this pull request as ready for review March 26, 2025 15:38
@KManolov3 KManolov3 changed the title [WIP] Feature/uepr 40 desktop feature parity Feature/uepr 40 desktop feature parity Mar 26, 2025
@@ -9,7 +9,7 @@ const libraries = require('./lib/libraries');

const ASSET_HOST = 'cdn.assets.scratch.mit.edu';
const NUM_SIMULTANEOUS_DOWNLOADS = 5;
const OUT_PATH = path.resolve('static', 'assets');
const OUT_PATH = path.resolve('static', 'fetched');
Copy link
Author

Choose a reason for hiding this comment

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

This was due to a strange issue, where if these assets existed in a folder named static/assets (which was created outside of the dist packaged by Electron, .asar in the case of linux), we were unable to resolve tutorial images (which are also in a static/assets folder, but inside the packaged dist.

loader: 'file-loader',
options: {
outputPath: 'static/assets/'
type: 'asset/resource',
Copy link
Author

Choose a reason for hiding this comment

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

For a reason I'm not completely sure of, this does not save files inside dist/renderer/static/assets (which file-loader previously did). The most plausible explanation seems to be that webpack is stricter on where it looks and it automatically excludes files in node_modules (although adding includePaths here does not have an effect).

This is why I am resolving to copying these assets from scratch-gui (with the CopyWebpackPlugin below).

I feel like I might be missing something on how these were previously output in the dist, but the current setup seems to work. We'll see soon if it causes issues on the other platforms.

Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

Amazing work. Do you feel like you've run a marathon? I'm sorry you now know so much about Electron and Webpack! ;)

If you're confident about how to resolve the README.md TODO related to making a new desktop release, I'd update that, but I think it's also fine to leave it until we're doing a real release. I'd prefer obviously outdated instructions (or no instructions at all) over new-but-untested instructions ;)

@@ -5,6 +5,7 @@ Scratch 3.0 as a standalone desktop application
## Developer Instructions

### Releasing a new version
# TODO: Update readme once scratch-desktop uses scratch-gui from an npm package
Copy link
Contributor

Choose a reason for hiding this comment

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

These instructions just start with step 2 now and skip 2.4-2.7, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that should be the case

@@ -45,3 +53,12 @@ appx:
nsis:
oneClick: false # allow user to choose per-user or per-machine
artifactName: "Scratch ${version} Setup.${ext}"
linux:
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

package.json Outdated
@@ -36,42 +39,52 @@
"@babel/plugin-transform-optional-chaining": "7.25.9",
"@babel/preset-env": "7.26.7",
"@babel/preset-react": "7.26.3",
"@scratch/scratch-gui": "^11.0.0-beta.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀 👨‍🎤 🎸 🕺 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants