Clean up language selector #2460
Conversation
This was added during source-academy#2444 for seemingly no reason.
Turns out the chapter list rendered has been used wrongly this whole time, resulting in an inability to filter. After fixing, a lot of the conditional rendering can be simplified. This is due to an additional attribute of SALanguage that was created: the main language that a sublanguage belongs to.
* Remove commented code * Remove console.log statement * Remove useless React fragment
* Use the language enum in the component test * Remove console.log from test file * Update snapshot
Removed unneeded exports. Also updated the default language selector to match the abstraction level of the chapter selector in the Playground.
Pull Request Test Coverage Report for Build 4835377873
💛 - Coveralls |
|
Before this PR, the existing behaviour was that the following languages:
Will only show up in the public deployment (and not Source Academy @ X). I've changed the behaviour such that Scheme and Python now also shows up in SA@X deployments. I believe @zhaojj2209 mentioned something about potential security vulnerabilities/XSS/etc. if we run Full JS/TS/HTML. @zhaojj2209 Can I check if there were any other reasons these languages were set to the public deployment only? I've made sure to maintain that same behaviour for SA@X deployments, as seen in: I think enabling Scheme and Python are fine (since they are not running "full" versions of the language). This also "fixes" #2459. What are your thoughts on this @martin-henz ? |
|
@s-kybound can I check on the difference between "Scheme §4" and "Full Scheme"? Would the abovementioned security issue be likely? @Fidget-Spinner @dylkaw likewise, are there plans for "Full Python"? |
|
No plans for Full Python. A full Python to JS transpiler would be a monumental project. |
Hi, "Scheme §4" and "Full Scheme" currently have no differences. "Scheme §4" only enables enough of Scheme to complete SICP Chapter 4, while "Full Scheme" will, in future, enable features like macros and first-class continuations. I don't think there will be security issues that could arise from this. |
Full JS/TS should not ever be deployed to the NUS version because they make use of |
* Remove unused dependency This was added during source-academy#2444 for seemingly no reason. * Simplify ChapterSelect state update logic * Fix sublanguage rendering logic and update types Turns out the chapter list rendered has been used wrongly this whole time, resulting in an inability to filter. After fixing, a lot of the conditional rendering can be simplified. This is due to an additional attribute of SALanguage that was created: the main language that a sublanguage belongs to. * Fix Scheme, Python not loading on SA @ X deploys * Clean up code * Remove commented code * Remove console.log statement * Remove useless React fragment * Refactor language names to use enum * Update test and snapshot * Use the language enum in the component test * Remove console.log from test file * Update snapshot * Migrate playground language state from to use enum * Remove commented code * Refactor sublanguage definitions * Update broken tests * Fix lint error * Improve language abstractions Removed unneeded exports. Also updated the default language selector to match the abstraction level of the chapter selector in the Playground.
5 participants
Description
Cleans up a bunch of code style and quality issues following #2444. Also closes #2459. Do look at commit history for more details.
Turns out the chapter list renderer has been used wrongly this whole time. Items were fetched from exported variables instead of the state. This made it hard to filter. Has since been fixed in 2a3801e.
Type of change
How to test
Checklist