Migrate from lodash to es-toolkit by AaravMalani · Pull Request #596 · source-academy/modules

AaravMalani · 2026-02-12T07:27:51Z

Description

Migrate from lodash to es-toolkit

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Note: ~~A couple of Github Actions tests fail, hence I'm making a draft PR till it gets resolved~~ Fixed, making it a normal PR

leeyi45

Left some comments. Also please address some of the linting errors. Otherwise, thanks for doing the migration work

lib/lintplugin/src/rules/__tests__/noBarrelImports.test.ts

.github/actions/src/__tests__/commons.test.ts

leeyi45 · 2026-02-12T13:23:57Z

Will close #557

leeyi45 · 2026-02-13T02:20:31Z

Btw, was there any particular reason why you're performing this migration? @AaravMalani

AaravMalani · 2026-02-18T07:40:32Z

Btw, was there any particular reason why you're performing this migration? @AaravMalani

I wanted to increase build times for py-slang. With the current setup, py-slang provides the entire lodash to the modules which it loads, so Rollup has to bundle it at build-time. It'd be more efficient for the modules to contain the functions they require, especially since most modules use one or two functions at max, and es-toolkit functions have a significantly tinier footprint than lodash functions (I think lodash-es bundled more than 10 different functions with just merge)

…to migrate-es-toolkit

Stale

RichDom2185

If @leeyi45 has further comments please do comment

.yarnrc.yml

RichDom2185 · 2026-02-26T18:18:42Z

eslint.config.js

      'prefer-const': ['warn', { destructuring: 'all' }],

      '@sourceacademy/default-import-name': ['warn', { path: 'pathlib' }],
-      '@sourceacademy/no-barrel-imports': ['error', ['lodash']],


RichDom2185 · 2026-02-26T18:19:00Z

package.json

+    "react": "^18.3.1",
+    "react-dom": "^18.3.1",


Revert

Suggested change

"react": "^18.3.1",

"react-dom": "^18.3.1",

I think the tests were failing without react in package.json for some reason

docs/src/lib/lintplugin/2-rules.md

RichDom2185 · 2026-02-26T18:24:26Z

.github/actions/src/__tests__/lockfiles.test.ts

+    expect(utils.extractPackageName('es-toolkit@npm:^1.44.0'))
+      .toEqual('es-toolkit');


Why do we need to update these?

RichDom2185 · 2026-02-26T18:24:44Z

.github/actions/src/__tests__/sample_why.txt

I wasn't sure what the purpose of the file was, considering I can't find any uses for it in the repo

RichDom2185 · 2026-02-26T18:24:48Z

.github/actions/src/info/__tests__/index.test.ts

Co-authored-by: Richard Dominick <34370238+RichDom2185@users.noreply.github.com>

sentry · 2026-03-02T07:39:37Z

src/bundles/robot_simulation/src/controllers/utils/mergeConfig.ts

    return defaultConfig;
  }
-  return _.merge({ ...defaultConfig }, userConfig);
+  return merge({ ...defaultConfig }, userConfig);


Bug: The mergeConfig function uses a shallow copy, which allows the merge utility to mutate nested objects within the original defaultConfig.
_{Severity: MEDIUM}

Suggested Fix

To prevent unintended mutations, perform a deep copy of the defaultConfig object before passing it to the merge function. This ensures that nested objects are also duplicated, breaking the reference to the original configuration and preserving its integrity.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: src/bundles/robot_simulation/src/controllers/utils/mergeConfig.ts#L8 Potential issue: The `mergeConfig` function uses a shallow copy (`{ ...defaultConfig }`) to protect the default configuration from being modified. However, this protection is insufficient for nested objects. When a configuration object contains nested objects (e.g., a `THREE.Color` instance), the shallow copy shares references to these nested objects. The `merge` function, which modifies its target object in place, will then mutate these shared nested objects, leading to unintended mutation of the original `defaultConfig`. The existing unit test for mutation only covers flat objects and does not detect this issue with nested structures.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

Migrate from lodash to es-toolkit

d5f7dd3

leeyi45 previously requested changes Feb 12, 2026

View reviewed changes

lib/lintplugin/src/rules/__tests__/noBarrelImports.test.ts Outdated Show resolved Hide resolved

.github/actions/src/__tests__/commons.test.ts Outdated Show resolved Hide resolved

Revert noBarrelImports tests + mock memoize function

7108867

AaravMalani added 2 commits February 18, 2026 16:30

Fix weird dependency aliasing

9540a78

Lint files

870b6ba

RichDom2185 marked this pull request as ready for review February 18, 2026 17:05

RichDom2185 and others added 4 commits February 19, 2026 01:10

Merge branch 'master' of https://github.com/source-academy/modules in…

d0f3bdf

…to migrate-es-toolkit

Fix lockfile post-merge

40e4e05

Fix missing packages

0dfdaf6

Fix omit change to es-toolkit + Add react to base module

00bbd27

AaravMalani requested review from RichDom2185 and leeyi45 February 19, 2026 13:50

Merge branch 'master' into migrate-es-toolkit

81e4198

RichDom2185 reviewed Feb 26, 2026

View reviewed changes

AaravMalani and others added 2 commits March 2, 2026 15:35

Update .yarnrc.yml

4cfe7d0

Co-authored-by: Richard Dominick <34370238+RichDom2185@users.noreply.github.com>

Update docs/src/lib/lintplugin/2-rules.md

80e3ae4

Co-authored-by: Richard Dominick <34370238+RichDom2185@users.noreply.github.com>

sentry bot reviewed Mar 2, 2026

View reviewed changes

		expect(utils.extractPackageName('es-toolkit@npm:^1.44.0'))
		.toEqual('es-toolkit');

Conversation

AaravMalani commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

leeyi45 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leeyi45 commented Feb 12, 2026

Uh oh!

leeyi45 commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaravMalani commented Feb 18, 2026

Uh oh!

RichDom2185 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RichDom2185 Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

RichDom2185 Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

AaravMalani Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RichDom2185 Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

RichDom2185 Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

AaravMalani Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

RichDom2185 Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

sentry bot Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AaravMalani commented Feb 12, 2026 •

edited

Loading

leeyi45 commented Feb 13, 2026 •

edited

Loading