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 @embroider/test-setup to test against embroider #989

Merged
merged 2 commits into from
Aug 16, 2021

Conversation

thoov
Copy link
Member

@thoov thoov commented Feb 12, 2021

Adds 2 test cases against embroider for ember-try.

@stefanpenner
Copy link
Member

I've rebased this, the failing tests are due to the ember-cli-typescript used in this project, which as implemented is not compatible with embroider.

I intend to address next.

package.json Show resolved Hide resolved
@stefanpenner stefanpenner force-pushed the test-embroider branch 5 times, most recently from 67a1f45 to ba243c1 Compare August 6, 2021 15:37
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@stefanpenner stefanpenner force-pushed the test-embroider branch 16 times, most recently from 3be3811 to a2e470e Compare August 9, 2021 21:09
rwjblue
rwjblue previously requested changes Aug 9, 2021
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Can you confirm:

  • rm API.md && yarn docs generates the current API.md (e.g. has no diff)
  • running a server works (file changes are detected, and cause a reload, tests re-run, etc)

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@stefanpenner
Copy link
Member

stefanpenner commented Aug 10, 2021

A small bonus:

before:

> rm API.md && time yarn docs
Done in 22.35s.
________________________________________________________
Executed in   23.28 secs   fish           external
   usr time   23.74 secs   59.00 micros   23.74 secs
   sys time    2.56 secs  671.00 micros    2.56 secs

after:

> rm API.md && time yarn docs
Done in 1.37s.

________________________________________________________
Executed in    1.54 secs   fish           external
   usr time  1385.11 millis   68.00 micros  1385.04 millis
   sys time  209.11 millis  760.00 micros  208.35 millis

And API.md resulting is identical.

new MergeTrees([input, tsTree]),
'addon-test-support:merged'
);
if (this.isDevelopingAddon()) {
Copy link
Member

Choose a reason for hiding this comment

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

@rwjblue this simplifies development, quickly removing the need to run a separate "TS build" process.
I wouldn't consider this a final product, but demonstrates the viability "TS in dev + precompiled on publish approach"

When you have time, we should discuss

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to understand this more too. I'm not entirely sure what you mean when you refer to "TS in dev + precompiled on publish approach", because that describes what's done today - we use TS in dev, and precompile for releases.

Copy link
Member

@stefanpenner stefanpenner Aug 12, 2021

Choose a reason for hiding this comment

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

the comment was more based on an intermediate step + discussion with rob.

Functionally this appears the same as prior, but no longer requires ember-cli-typescript, which changed the above in the most recent versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

As yes, I remember this. This was the removal of the incremental-typescript-compiler from 1.5.0 -> 2.0.0. Makes sense.

@stefanpenner
Copy link
Member

GH actions appears to be queued up... Will poke back later...

.github/workflows/ci-build.yml Show resolved Hide resolved
.github/workflows/ci-build.yml Show resolved Hide resolved
new MergeTrees([input, tsTree]),
'addon-test-support:merged'
);
if (this.isDevelopingAddon()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to understand this more too. I'm not entirely sure what you mean when you refer to "TS in dev + precompiled on publish approach", because that describes what's done today - we use TS in dev, and precompile for releases.

package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
thoov and others added 2 commits August 16, 2021 17:08
- run floating deps + try scenarios in node 12. This is to minimize frustrating test failures due to package drift, and to allow for testing embroider in CI. While officially supporting node 10 when not using embroider
- upgrade all other viable deps
- move to simple tsc -b approach
- only run embroider-safe, as embroider-optimized is not yet compatible with our tests
@stefanpenner stefanpenner merged commit 384a589 into emberjs:master Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants