Skip to content

Conversation

@lifeart
Copy link
Contributor

@lifeart lifeart commented Sep 5, 2020

image
image

@lifeart
Copy link
Contributor Author

lifeart commented Sep 5, 2020

I'm getting runtime error if I use npm
image

With yarn all ok

@lifeart lifeart marked this pull request as ready for review September 5, 2020 10:39
@lifeart lifeart changed the title [WIP] add glimmer2 benchmark add glimmer2 benchmark Sep 5, 2020
@krausest
Copy link
Owner

Sorry, doesn't work for me I'm getting the following error:

app.bundle.js:1 Uncaught Error: The `@glimmer/validator` library has been included twice in this application. It could be different versions of the package, or the same version included twice by mistake. `@glimmer/validator` depends on having a single copy of the package in use at any time in an application, even if they are the same version. You must dedupe your build to remove the duplicate packages in order to prevent this error.
    at Module.<anonymous> (app.bundle.js:1)
    at s (app.bundle.js:1)
    at app.bundle.js:1
    at app.bundle.js:1

@krausest krausest removed the merging started merging started (no more updates please) label Sep 10, 2020
@lifeart
Copy link
Contributor Author

lifeart commented Sep 13, 2020

@krausest is it possible to add yarn support to benchmarks?
Trying to force resolutions for npm, but no luck.
For now, I suppose to wait for glimmerjs/glimmer.js#303 (likely any new release will fix it)

@krausest
Copy link
Owner

There once was yarn support, but it caused some issues like #182
I‘ll try to build with yarn and post the results here and then I‘ll come back to answer the question.

@krausest
Copy link
Owner

@lifeart Building with yarn worked. Here are the results (vanillajs and glimmer-2 were rerun. vanillajs looks reasonable):

I'd rather prefer to wait for a fix for npm. We've had a few nasty bugs (or at least inconsistencies from npm) in the past and the version lookup would have to be fixed.

@lifeart
Copy link
Contributor Author

lifeart commented Sep 16, 2020

@krausest thank you for this run! I Agree, with your concerns. I will convert MR to draft and will appear it back once new versions will be released (with fixed npm)

@lifeart lifeart marked this pull request as draft September 16, 2020 20:15
@krausest krausest added the draft label Sep 26, 2020
@lifeart lifeart marked this pull request as ready for review October 25, 2020 10:16
@lifeart
Copy link
Contributor Author

lifeart commented Oct 25, 2020

@krausest now it's alive for me

@krausest krausest added merging started merging started (no more updates please) and removed draft labels Nov 1, 2020
@krausest krausest merged commit 47a51ad into krausest:master Nov 1, 2020
@krausest
Copy link
Owner

krausest commented Nov 1, 2020

This works fine. #800 holds for this implementation too.

@lifeart lifeart deleted the glimmer-2 branch November 1, 2020 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merging started merging started (no more updates please)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants