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

Bundle stdlib runtime for playground #7255

Merged
merged 2 commits into from
Jan 25, 2025

Conversation

tsnobip
Copy link
Contributor

@tsnobip tsnobip commented Jan 25, 2025

To load stdlib JS files in the playground, we need to bundle them and upload them to the CDN.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: 5af9419 Previous: e1b7fb7 Ratio
Parse RedBlackTree.res - time/run 1.36702038 ms 1.2123143266666667 ms 1.13
Parse Napkinscript.res - time/run 42.357428 ms 39.28006235333333 ms 1.08
Parse HeroGraphic.res - time/run 6.391031979999999 ms 5.13472718 ms 1.24

This comment was automatically generated by workflow using github-action-benchmark.

@@ -75,7 +75,7 @@ function buildCompilerCmij() {
}

e(
`find ${rescriptLibOcamlFolder} -name "*.cmi" -or -name "*.cmj" | xargs -n1 basename | xargs js_of_ocaml build-fs -o ${cmijFile} -I ${rescriptLibOcamlFolder}`,
`find ${rescriptLibOcamlFolder} -name "*.cmi" -or -name "*.cmj" | xargs -n1 basename | xargs js_of_ocaml build-fs -o ${cmijFile} -I ${rescriptLibOcamlFolder}`
Copy link
Member

Choose a reason for hiding this comment

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

I really thought this line would need to be changed, but I am glad this is just formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fhammerschmidt would you like the formatting noise to be removed?

@@ -31,7 +31,7 @@ let string_of_module_id_in_browser (x : Lam_module_ident.t) =
match x.kind with
| External {name} -> name
| Runtime | Ml ->
"./stdlib/" ^ Ext_string.uncapitalize_ascii x.id.name ^ ".js"
"./stdlib/" ^ x.id.name ^ ".js"
Copy link
Member

@fhammerschmidt fhammerschmidt Jan 25, 2025

Choose a reason for hiding this comment

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

are you sure this can go away for both Ml and Runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is only used in the playground so it should be safe. It was invalid before.

Copy link
Member

@fhammerschmidt fhammerschmidt left a comment

Choose a reason for hiding this comment

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

looks good, just a small comment/question

Copy link
Member

@fhammerschmidt fhammerschmidt left a comment

Choose a reason for hiding this comment

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

Clarified comments on Discord, good to go! 🎉

@fhammerschmidt fhammerschmidt enabled auto-merge (squash) January 25, 2025 21:02
@fhammerschmidt fhammerschmidt merged commit b0383bd into master Jan 25, 2025
20 checks passed
tsnobip added a commit that referenced this pull request Jan 27, 2025
tsnobip added a commit that referenced this pull request Jan 27, 2025
fhammerschmidt pushed a commit that referenced this pull request Jan 27, 2025
cknitt added a commit that referenced this pull request Feb 1, 2025
* backport bundle stdlib JS files for playground (#7255)

* replace deprecated pipes with shlex

* update setup-ocaml

* use node 18

* add rollup linux as an optional dependency

* use ubuntu-24.04 and ubuntu-24.04-arm

* backport fix for hanging tests (#6667)

* update artifacts.txt

* backport: CI: don't run npm pack twice (#6923)

* CI: don't run npm pack twice

* Add typedefs for PackOutput

* Update comments

* update artifacts

* Fix CHANGELOG.md

---------

Co-authored-by: Christoph Knittel <ck@cca.io>
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.

2 participants