FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(stack-blitz-tests): revert enabling Ivy as it broke harness examples by Splaktar · Pull Request #1009 · angular/material.angular.io · GitHub

This repository was archived by the owner on Jan 4, 2026. It is now read-only.
  1. angular
  2. material.angular.io

Repository navigation

fix(stack-blitz-tests): revert enabling Ivy as it broke harness examples - #1009

Merged
crisbeto merged 1 commit into
angular:masterfrom
DevIntent:fix-harness-demos-remove-tslint
Jul 29, 2021
ConversationCommits (1)ChecksFiles changed
Merged

fix(stack-blitz-tests): revert enabling Ivy as it broke harness examples#1009
crisbeto merged 1 commit into
angular:masterfrom
DevIntent:fix-harness-demos-remove-tslint

Conversation

Splaktar commented Jun 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • remove tslint and codelyzer from examples
  • update jasmine dependencies
  • remove unused material-module.ts from test assets
  • add jasmine-core to package.json in stack-blitz-tests/
  • disable noImplicitAny in stack-blitz-tests' tsconfig.spec.json since
    jasmine-core/lib/jasmine-core/jasmine.js imported in test/jasemine-setup.ts
    is not compatible

Fixes #997

The stack-blitz-tests/ asset files are used for generating the StackBlitz examples for the test harnesses. In a past PR, we set them along with all of our StackBlitz assets to use Ivy. This worked fine for our component examples, but broke our Harness examples as mentioned in #997 (comment). This PR reverts that change by switching the Harness examples to use View Engine in StackBlitz. The component examples will continue to use Ivy in StackBlitz.

Additionally, this PR fixes some issues blocking the use of Ivy with the Harness examples (since jasmine-core/lib/jasmine-core/jasmine.js doesn't build in strict mode with noImplicitAny enabled). It also updates and removes some other dependencies.

Splaktar requested review from amysorto and andrewseguin June 7, 2021 01:20
google-cla Bot added the cla: yes label Jun 7, 2021
Splaktar mentioned this pull request Jun 7, 2021
Closed

andrewseguin commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

The material-module.ts is used by the stackblitz writer.

It's not clear why Ivy should be disabled - is there something that the framework needs to fix in order for us to turn it back on?

Splaktar commented Jun 7, 2021

Copy link
Copy Markdown
Contributor Author

This workaround from Pete might work to keep Ivy enabled:
angular/angular#41999 (comment)

Splaktar commented Jun 7, 2021

Copy link
Copy Markdown
Contributor Author

@andrewseguin not by assets/stack-blitz-tests, only by assets/stack-blitz. I.e. TEMPLATE_FILES uses it but TEST_TEMPLATE_FILES does not.

Splaktar commented Jun 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@andrewseguin also after fixing the noImplicitAny error, with Ivy enabled, you get this in StackBlitz

Error in src/app/input-harness-example.spec.ts (14:1)
Cannot find name 'describe'. Do you need to install type definitions for a test runner?
Try `npm i --save-dev @types/jest` or `npm i --save-dev @types/mocha`.

Disabling Ivy resolves this. It's unclear why there are references to Jest or Mocha when Jasmine is being used.

gkalpak commented Jun 7, 2021

Copy link
Copy Markdown
Member

I don't know why the error happens, but the error message comes from TypeScript itself: https://github.com/microsoft/TypeScript/blob/9906092db25aae8bd6f6b04aef5e8906251fcf13/src/compiler/diagnosticMessages.json#L2489 🤷

Splaktar commented Jun 7, 2021

Copy link
Copy Markdown
Contributor Author

This workaround from Pete might work to keep Ivy enabled:
angular/angular#41999 (comment)

@crisbeto unfortunately, that's not working for me in https://stackblitz.com/edit/jasmine-ivy-workaround?file=tsconfig.app.json.

crisbeto commented Jun 7, 2021

Copy link
Copy Markdown
Member

Would everything else work if we put a @ts-ignore just on this one import?

Splaktar commented Jun 7, 2021

Copy link
Copy Markdown
Contributor Author

@crisbeto that gets us past the noImplicityAny and on to the TypeScript error above about describe not being defined.

Splaktar requested review from wagnermaciel and removed request for andrewseguin June 10, 2021 16:47
fix(stack-blitz-tests): revert enabling Ivy as it broke harness examples
769a36c 
- remove tslint and codelyzer from examples
- update jasmine dependencies
- remove unused material-module.ts from test assets
- add jasmine-core to package.json in stack-blitz-tests/
- disable `noImplicitAny` in stack-blitz-tests' tsconfig.spec.json since
  `jasmine-core/lib/jasmine-core/jasmine.js` imported in `test/jasemine-setup.ts`
   is not compatible

Fixes angular#997
Splaktar force-pushed the fix-harness-demos-remove-tslint branch from b901073 to 769a36c Compare July 19, 2021 18:36
Splaktar changed the title fix(stack-blitz-tests): disable Ivy to fix harness examples fix(stack-blitz-tests): revert enabling Ivy as it broke harness examples Jul 19, 2021
crisbeto mentioned this pull request Jul 21, 2021
Closed
mmalerba approved these changes Jul 29, 2021
View reviewed changes

mmalerba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

crisbeto merged commit e190dc4 into angular:master Jul 29, 2021
Splaktar deleted the fix-harness-demos-remove-tslint branch August 6, 2021 17:42
Splaktar mentioned this pull request Aug 6, 2021
Open

Splaktar commented Aug 6, 2021

Copy link
Copy Markdown
Contributor Author

Issue #1037 tracks properly re-enabling Ivy and ensuring that the Harness Examples work.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

amysorto Awaiting requested review from amysorto
wagnermaciel Awaiting requested review from wagnermaciel
1 more reviewer
mmalerba mmalerba approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes P1: urgent pr: merge ready

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug(components with test harness example): Missing dependencies in StackBlitz examples

5 participants

Back | FazBrowse Home | New Git URL