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

WIP feat(test): switch from aurelia-pal-nodejs to aurelia-pal-browser for jest setup #1019

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@huochunpeng
Copy link
Member

huochunpeng commented Jan 16, 2019

jest by default does simulate browser environment. It's unnecessary to use pal-nodejs (which brings in jsdom but doesn't pollute global vars). Using pal-browser with default jest simulation has extra benefit: be more compatible with 3rd party libs which expects global vars.

cloese #1018

@huochunpeng huochunpeng force-pushed the huochunpeng:simplify-jest branch from 010a001 to 7040cab Jan 16, 2019

@huochunpeng huochunpeng changed the title feat: switch from aurelia-pal-nodejs to aurelia-pal-browser for jest setup feat(test): switch from aurelia-pal-nodejs to aurelia-pal-browser for jest setup Jan 16, 2019

@huochunpeng

This comment has been minimized.

Copy link
Member Author

huochunpeng commented Jan 16, 2019

@shahabganji let's know whether it works for you. Thx.

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Jan 23, 2019

@huochunpeng What is the easiest way for @shahabganji to test this out and give us feedback on whether it will work better than the current setup?

@huochunpeng

This comment has been minimized.

Copy link
Member Author

huochunpeng commented Jan 23, 2019

@shahabganji was about to test it out, but seems didn't get time on it.
@EisenbergEffect anyone with a non-trivial webpack app could try this out. I only tried on tiny app.

@shahabganji

This comment has been minimized.

Copy link

shahabganji commented Jan 23, 2019

@EisenbergEffect , @huochunpeng Sorry I have not seen this, I'll check it and will let you know within a couple of hours 🙏

@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Jan 23, 2019

No need to apologize @shahabganji We appreciate you jumping in whenever you can.

@huochunpeng

This comment has been minimized.

Copy link
Member Author

huochunpeng commented Jan 23, 2019

@shahabganji this is the same fix as the one posted on your issue.

@shahabganji

This comment has been minimized.

Copy link

shahabganji commented Jan 23, 2019

@huochunpeng

@shahabganji this is the same fix as the one posted on your issue.

I did not use them, since both of the approaches I've mentioned there also worked, however, I'll check your changes to make sure it works properly too 👍

@Vheissu

This comment has been minimized.

Copy link
Member

Vheissu commented Jan 23, 2019

I love the idea of removing the need for Node.js in the Jest tests, however, I am unable to get this working with TypeScript/Jest and Webpack based app. Any test that requires staging a component (like a modal) fails with some weird errors @huochunpeng check the Discord when you get some time.

@shahabganji

This comment has been minimized.

Copy link

shahabganji commented Jan 23, 2019

@EisenbergEffect, I checked the changes @huochunpeng had made on the same repository I was working on, and jest just works seamlessly. Worth mentioning that I am using Typescript. This is an in progress project, as it gets bigger and bigger I'll update the team with any issues if happened.

@Vheissu We also used this setting on aurelia-toolbelt/experimental based on webpack and typescript, cypress have been installed too for e2e test, and we have no problem running the basics. That might be helpful. Can you also let us know about the issue you are faced?

shahabganji added a commit to aurelia-toolbelt/aurelia-toolbelt that referenced this pull request Jan 23, 2019

reconfigure( jest )
Change some configuration for jest based on @huochunpeng 's suggestions [here](aurelia/cli#1019) and [here](aurelia/cli#1018 (comment))

@huochunpeng huochunpeng changed the title feat(test): switch from aurelia-pal-nodejs to aurelia-pal-browser for jest setup WIP feat(test): switch from aurelia-pal-nodejs to aurelia-pal-browser for jest setup Jan 23, 2019

@huochunpeng

This comment has been minimized.

Copy link
Member Author

huochunpeng commented Jan 23, 2019

Waiting for a fix on aurelia-bootstrapper to remove the runtime dependency of aurelia-pal-nodejs.

huochunpeng added a commit to huochunpeng/bootstrapper that referenced this pull request Jan 24, 2019

fix: avoid unnecessary pal loading when pal is already initialized
aurelia-pal could be initialized before bootstrapping, such as test setup. This is to help aurelia/cli#1019 to avoid loading aurelia-pal-nodejs when aurelia-pal-browser is already loaded.
@huochunpeng

This comment has been minimized.

Copy link
Member Author

huochunpeng commented Jan 24, 2019

  • update aurelia-bootstrapper to avoid loading aurelia-pal-nodejs aurelia/bootstrapper#70
  • need a polyfill for MutationObserver in jest(jsdom) env.
    • either move aurelia-pal-nodejs/src/polyfills/mutation-observer.ts to aurelia-polyfills.
    • or load the polyfill in test/jest-pretest.js. It means the app still has aurelia-pal-nodejs in dependencies.
    • use jsdom v13 to get latest MutationObserver polyfill.
  • @Vheissu has an unsolved issue: The class property of a object ([object HTMLDivElement]) cannot be assigned.
@EisenbergEffect

This comment has been minimized.

Copy link
Member

EisenbergEffect commented Jan 24, 2019

@huochunpeng I haven't looked in a while but is that MutationObserver polyfill usable in a browser context? I didn't think it was. I thought it was very JSDom-specific, in which case we wouldn't want to move it to the polyfills library, for example.

@huochunpeng

This comment has been minimized.

Copy link
Member Author

huochunpeng commented Jan 24, 2019

OK, I guess we only have option2, put it in jest-pretest.

@huochunpeng huochunpeng force-pushed the huochunpeng:simplify-jest branch from 7040cab to 67f7f16 Jan 24, 2019

@huochunpeng

This comment has been minimized.

Copy link
Member Author

huochunpeng commented Jan 30, 2019

Note: latest jsdom v13.2.0 has added MutationObserver!

feat(test): switch from aurelia-pal-nodejs to aurelia-pal-browser for…
… jest setup

jest by default does simulate browser environment. It's unnecessary to use pal-nodejs (which brings in jsdom but doesn't pollute global vars). Using pal-browser with default jest simulation has extra benefit: be more compatible with 3rd libs which expects global vars.
Using jest-environment-jsdom-thirteen for MutationObserver in jsdom v13.

cloese #1018

@huochunpeng huochunpeng force-pushed the huochunpeng:simplify-jest branch from 67f7f16 to 0a63905 Feb 6, 2019

@huochunpeng

This comment has been minimized.

Copy link
Member Author

huochunpeng commented Feb 6, 2019

@Vheissu could you try one more time with jest-environment-jsdom-thirteen?

huochunpeng added a commit to huochunpeng/pal-nodejs that referenced this pull request Feb 6, 2019

feat: use jsdom v13.2.0 to replace internal MutationObserver polyfill
Dropped internal MutationObserver polyfill in favour of jsdom v13.2.0.
Also simpified and enhanced global vars creations for simulating browser environment, thank browser-env. It improves compatibility with other 3rd party libs in user's app, for example, global vars navigator/sessionStorage/localStorage are now available.
This also provides an alternative way to improve the compatibility of test setup in jest, comparing to aurelia/cli#1019.

huochunpeng added a commit to huochunpeng/pal-nodejs that referenced this pull request Feb 6, 2019

feat: use jsdom v13.2.0 to replace internal MutationObserver polyfill
Dropped internal MutationObserver polyfill in favour of jsdom v13.2.0.
Also simpified and enhanced global vars creations for simulating browser environment, thank browser-env. It improves compatibility with other 3rd party libs in user's app, for example, global vars navigator/sessionStorage/localStorage are now available.
This also provides an alternative way to improve the compatibility of test setup in jest, comparing to aurelia/cli#1019.

huochunpeng added a commit to huochunpeng/pal-nodejs that referenced this pull request Feb 6, 2019

feat: use jsdom v13.2.0 to replace internal MutationObserver polyfill
Dropped internal MutationObserver polyfill in favour of jsdom v13.2.0.
Also simpified and enhanced global vars creations for simulating browser environment, thank browser-env. It improves compatibility with other 3rd party libs in user's app, for example, global vars navigator/sessionStorage/localStorage are now available.
This also provides an alternative way to improve the compatibility of test setup in jest, comparing to aurelia/cli#1019.

huochunpeng added a commit to huochunpeng/pal-nodejs that referenced this pull request Feb 6, 2019

chore: use jsdom v13.2.0 to replace internal MutationObserver polyfill
Dropped internal MutationObserver polyfill in favour of jsdom v13.2.0.
Also simpified and enhanced global vars creations for simulating browser environment, thank browser-env. It improves compatibility with other 3rd party libs in user's app, for example, global vars navigator/sessionStorage/localStorage are now available.
This also provides an alternative way to improve the compatibility of test setup in jest, comparing to aurelia/cli#1019.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment