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

make @glimmer/compiler runable in browser #1311

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

izelnakri
Copy link

@izelnakri izelnakri commented May 20, 2021

Removes node.js crypto API usage so the compiler can be used in all JS runtimes. Instead, now we simply increment the template default id for each template for compilation.

Removes node.js crypto API usage so the compiler can be used in all
JS runtimes. Instead, now we simply increment the template default id
for each template compilation.
@izelnakri izelnakri force-pushed the make-glimmer-compiler-for-browser branch from cb4959f to e1cd954 Compare May 20, 2021 21:43
@rwjblue
Copy link
Member

rwjblue commented Sep 23, 2021

Hmmm, I don't think this should be needed. It is definitely possible to use @glimmer/compiler in the browser without hitting this require('crypto') code (all of Ember's own tests do this and run in the browser).

Maybe you can explain what you ran into specifically?

@izelnakri
Copy link
Author

izelnakri commented Sep 23, 2021

Hi @rwjblue thanks for your response!

It's been 3 months since I created the issue so I'll need to remember where I exactly need this 😅

However I know that I needed this change absolutely when I was building @emberx/component experimental packages after many trials to build the base glimmer compiler/packages for the browser:

I do a similar code adjustment here automated:
https://github.com/izelnakri/emberx/blob/main/scripts/build-libraries.js#L8 (These build scripts automatically adjust the npm downloaded packages so I dont have to manually adjust them on each npm install)

Currently ember and glimmer tests run through ember specific build systems including src package builds, thus they are not directly usable in browser, they go through certain adjustments in broccoli, build systems create its browser runnable dists. Thats why these type of limits/errors are currently not possible to be discoverable directly by the glimmer and ember dev teams, so would be great if we test them in browser via some generic build system like esbuild. (I've also had to built qunitx project just for this purpose, allows me to run qunit in node and browser with esbuild, the most generic build system possible I could find).

I've spent a lot of time on emberx experiment, at the time of the experiment these changes/PRs were needed but I dont remember where the error came from, it could be that I was running this file on node.js ESM mode during SSR(ESM support should be an requirement again for glimmer in my view) and this line produces runtime error: https://github.com/glimmerjs/glimmer-vm/pull/1311/files#diff-0f1dcb5fb927ad508600ddf7eb57661cb56858c1d26ec2bcd02b352a6baf82f4L27

This runtime error occurs because node.js doesn't provide module and require objects on ESM mode.

Currently I'm busy working on an ember-data alternative for directly node.js/esm compliant glimmer app framework(so no compilers/cli build system for dev, perhaps one only needed to remove the glimmer runtime in production mode to remove bytes & make it AOT instead of JIT), I can reproduce the exact error once Im back to emberx experiment which is almost finished, will be a successful experiment when tests are done, was able to get a universal framework/library running without a build system however misses a data management library so I'm also experimenting with a ember-data alternative.

Again I'm not sure where this specific error came from but the scenario/runtime error above is a valid scenario. Based on node.js and esbuild errors on emberx I created this type of scripts(first link), so I could eventually eliminate these code adjustment/automation scripts with needed adjustments/PRs on the related dependencies.

@rwjblue
Copy link
Member

rwjblue commented Sep 27, 2021

@izelnakri - Thanks for your response! I totally agree with you, I'd like to see progress on that front (help absolutely welcome!!).

Specifically, I'd like to:

  • Change our build output to be ESM (instead of separate builds for CJS and various ES compat levels)
  • Stop publishing the typescript files, and instead only publish the .d.ts files
  • Update Ember's build pipeline to consume this much more normally, just like you would have needed to in your EmberX setup.

IMHO, this makes things a bit nicer/easier for Embroider too (one of my objectives is to get ember-source building properly under Embroider's Webpack builds without requiring so much cusomization; which would unlock quite a bit better shaking and whatnot).

@izelnakri
Copy link
Author

Hi @rwjblue , I'm definitely interested in helping out for these issues. I suspect I'll have some time in 3 weeks for contributions. We can chat/plan it over DMs, you can reach me: izelnakri#8658 on discord, @izelnakri on Telegram.

@NullVoxPopuli
Copy link
Contributor

I think @ef4 has an upcoming RFC that will make the compiler available at runtime (if imported), and could supersede this PR?

@ef4
Copy link
Contributor

ef4 commented Jan 29, 2022

No, this PR would probably be needed to make that idea work anyway.

@ef4
Copy link
Contributor

ef4 commented Jan 29, 2022

But also, the compiler definitely runs in the browser during ember's test suite already, so I don't know the details of what this is fixing.

@lifeart
Copy link
Contributor

lifeart commented Dec 5, 2023

I think it's a good improvement anyway.

Having incremental id's:

  • less bundle size
  • easy to debug and understand

In addition, it's -1 require usage.

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.

None yet

5 participants