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

Add Ember #2438

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add Ember #2438

wants to merge 18 commits into from

Conversation

NullVoxPopuli
Copy link

@NullVoxPopuli NullVoxPopuli commented Sep 28, 2024

Resolves #237

  • meta
    • no open issues regarding web-component compatibility
    • write summary
    • define expected results
  • Components' Implementation
  • Basic Tests Pass
  • Advanced Tests Pass
  • [ ] Integrate into reporting / infra tests are manually ran?

Of note, ember currently uses Vite, not webpack

old notes , so we have to make some tweaks to shoehorn a vite build into this repo's way of using webpack -- shouldn't be too bad tho.

Copy link

google-cla bot commented Sep 28, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Author

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

self-review -- need to make some changes

@NullVoxPopuli NullVoxPopuli changed the title WIP: Add Ember Add Ember Mar 6, 2025
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review March 6, 2025 15:38
@NullVoxPopuli
Copy link
Author

ready for review <3

Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

This looks significantly different from the other tests.

We use a (very jank) karma config that records test results and compares them against the expected results, in order to maintain a number of invariants, e.g. that we're running the same number of tests across all libraries.

Having a custom test stack across different libraries will impose a maintenance burden that I'm not super excited about. Would it be possible to compile this code to standard JS so that it runs in the same karma config as the other libraries?

<script src="/@embroider/core/vendor.js"></script>
<script type="module">
import Application from "#src/app";
import environment from "#config";
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere, what do these nonstandard import specifiers that start with # mean? What part of the build system makes them work? Could you instead write javascript here, with ./dist/config.js or similar?

Copy link
Author

Choose a reason for hiding this comment

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

This is just how package.json#imports works.

A super helpful feature when you enable type=module.

Copy link
Author

Choose a reason for hiding this comment

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

@NullVoxPopuli
Copy link
Author

NullVoxPopuli commented Mar 12, 2025

Would it be possible to compile this code to standard JS so that it runs in the same karma config as the other libraries?

I can build it, yeah - is there a test project made that i can point at built files?

I'll happily delete my port of the tests

We use a (very jank) karma config that records test results and compares them against the expected results, in order to maintain a number of invariants, e.g. that we're running the same number of tests across all libraries.

Back when i first did this, it looked like there was a bunch of test duplication everywhere? Is this still the case? I can double check next i revisit this repo.


UPDATE: i looked again -- all tests are copied to every library/framework and then modified pretty heavily in each one. (use of JSX vs not, different ways of rendering, instantiating, interacting, etc)

@rictic
Copy link
Contributor

rictic commented Mar 12, 2025

We use npm ci, so you'll need a package-lock.json file in the ember directory: https://github.com/webcomponents/custom-elements-everywhere/actions/runs/13702513171/job/38674192501?pr=2438

@NullVoxPopuli
Copy link
Author

NullVoxPopuli commented Mar 13, 2025

figured it out

with npm, how do you reference another package?

I've tried in package.json:

"webcomponents": "file:../__shared__/webcomponents",

and

"webcomponents": "link:../__shared__/webcomponents",

neither work,

# with link
❯ npm i
npm error code EUNSUPPORTEDPROTOCOL
npm error Unsupported URL Type "link:": link:../__shared__/webcomponents
npm error A complete log of this run can be found in: /home/nvp/.npm/_logs/2025-03-13T00_06_23_959Z-debug-0.log
# with file
❯ npm i
npm error Cannot read properties of null (reading 'matches')
npm error A complete log of this run can be found in: /home/nvp/.npm/_logs/2025-03-13T00_06_57_448Z-debug-0.log

I can't even add the folder

❯ npm add ../__shared__/webcomponents 
npm error Cannot read properties of null (reading 'matches')
npm error A complete log of this run can be found in: /home/nvp/.npm/_logs/2025-03-13T00_08_28_746Z-debug-0.log

Here are my versions

❯ npm -v
10.9.0

custom-elements-everywhere/libraries/ember on  add-ember [✘!] 
❯ node -v
v23.6.0

My issue was just that the repo was dirty, and npm wasn't handling it gracefully.

@NullVoxPopuli
Copy link
Author

Having a custom test stack across different libraries will impose a maintenance burden that I'm not super excited about.

It looks like all of the tests are custom already? what do you mean?

I checked, angular, svelte, solid, react, and they're all custom.

@NullVoxPopuli
Copy link
Author

Would it be possible to compile this code to standard JS

npm run build already does this 🎉

image

so that it runs in the same karma config as the other libraries?

these are also custom for every library/framework.

Would it make sense for the test runner move to a standard reporter such as TAP? so the test implementation doesn't matter so much? 🤔

mocha already supports TAP: https://mochajs.org/#tap
karma also supports TAP (via plugin): https://www.npmjs.com/package/karma-tap-reporter
and qunit (what ember uses) defaults TAP: https://qunitjs.com/api/reporters/tap/

image

text output of `npm test` using TAP
❯ npm test

> [email protected] test
> wireit

                               
> [email protected] test:ember /home/nvp/Development/OpenSource/custom-elements-everywhere/libraries/ember
> cross-env NODE_ENV=development vite build --mode test && ember test --path dist --config-file ./testem.cjs

vite v6.2.1 building for test...
<script src="/testem.js"> in "/tests/index.html" can't be bundled without type="module" attribute
<script src="/@embroider/core/vendor.js"> in "/tests/index.html" can't be bundled without type="module" attribute
<script src="/@embroider/core/test-support.js"> in "/tests/index.html" can't be bundled without type="module" attribute
<script src="/@embroider/core/vendor.js"> in "/index.html" can't be bundled without type="module" attribute
transforming...
✓ 262 modules transformed.
rendering chunks...
computing gzip size...
dist/@embroider/virtual/vendor.js             0.02 kB
dist/index.html                               0.46 kB │ gzip:   0.27 kB
dist/@embroider/virtual/test-support.js       0.46 kB
dist/tests/index.html                         0.83 kB │ gzip:   0.40 kB
dist/@embroider/virtual/vendor.css            0.00 kB │ gzip:   0.02 kB
dist/@embroider/virtual/test-support.css      0.00 kB │ gzip:   0.02 kB
dist/assets/tests-DajEaJoV.css               15.01 kB │ gzip:   3.89 kB
dist/assets/main-CX4QLNt2.js                  0.08 kB │ gzip:   0.10 kB
dist/assets/tests-C2MSmCUR.js               527.48 kB │ gzip: 117.60 kB
dist/assets/app-Ch1EEBFm.js               1,304.87 kB │ gzip: 330.32 kB

(!) Some chunks are larger than 500 kB after minification. Consider:
- Using dynamic import() to code-split the application
- Use build.rollupOptions.output.manualChunks to improve chunking: https://rollupjs.org/configuration-options/#output-manualchunks
- Adjust chunk size limit for this warning via build.chunkSizeWarningLimit.
✓ built in 3.40s
ok 1 Chrome 133.0 - [91 ms] - advanced support > attributes and properties: will pass array data as a property
ok 2 Chrome 133.0 - [36 ms] - advanced support > attributes and properties: will pass object data as a property
ok 3 Chrome 133.0 - [36 ms] - advanced support > attributes and properties: will pass object data to a camelCase-named property
ok 4 Chrome 133.0 - [42 ms] - advanced support > events: can declaratively listen to a lowercase DOM event dispatched by a Custom Element
ok 5 Chrome 133.0 - [41 ms] - advanced support > events: can declaratively listen to a kebab-case DOM event dispatched by a Custom Element
ok 6 Chrome 133.0 - [42 ms] - advanced support > events: can declaratively listen to a camelCase DOM event dispatched by a Custom Element
ok 7 Chrome 133.0 - [43 ms] - advanced support > events: can declaratively listen to a CAPScase DOM event dispatched by a Custom Element
ok 8 Chrome 133.0 - [42 ms] - advanced support > events: can declaratively listen to a PascalCase DOM event dispatched by a Custom Element
ok 9 Chrome 133.0 - [35 ms] - basic support > no children: can display a Custom Element with no children
ok 10 Chrome 133.0 - [43 ms] - basic support > with children: can display a Custom Element with children in a Shadow Root
ok 11 Chrome 133.0 - [38 ms] - basic support > with children: can display a Custom Element with children in a Shadow Root and pass in Light DOM children
ok 12 Chrome 133.0 - [46 ms] - basic support > with children: can display a Custom Element with children in the Shadow DOM and handle hiding and showing the element
ok 13 Chrome 133.0 - [36 ms] - basic support > attributes and properties: will pass boolean data as either an attribute or a property
ok 14 Chrome 133.0 - [37 ms] - basic support > attributes and properties: will pass numeric data as either an attribute or a property
ok 15 Chrome 133.0 - [36 ms] - basic support > attributes and properties: will pass string data as either an attribute or a property
ok 16 Chrome 133.0 - [45 ms] - basic support > events: can imperatively listen to a DOM event dispatched by a Custom Element
ok 17 Chrome 133.0 - [0 ms] - ember-qunit: Ember.onerror validation: Ember.onerror is functioning properly

1..17
# tests 17
# pass  17
# skip  0
# todo  0
# fail  0

# ok
✅ Ran 1 script and skipped 0 in 6.5s.

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.

Add Ember.js
2 participants