fix(package): update core-js to version 3.0.0 #246

oktav777 · 2019-03-22T13:37:48Z

Closes #244
Closes #243

thymikee · 2019-03-22T13:41:55Z

package.json

@@ -19,6 +19,7 @@
  "peerDependencies": {
    "@angular/core": ">=2.0.0",
    "@angular/platform-browser-dynamic": ">=2.0.0",
+    "core-js": "^3.0.0",


This would be a breaking change, while this preset works with core-js@2. Can we make it "core-js": ">=2.0.0 < 4.0.0", for now and adjust the setupJest to try imports from core-js v2 and v3?

I am thinking to try/catch the require() functions. Will that be a good idea?

If we want to support both, this is the only solution I can see for now :D

I got a small issue :)
How am I supposed to test the changes? I can't make tests fail.

I deleted ./setupJest.js, ran npm run prepare, and then both npm run test are green in ./ and ./example

We'd need to have 2 projects, which is slow to test unfortunately. What you can do is to import ./setupJest.js from a new test file and mock require there to behave appropriately for v2 and v3

I think a require-mock is appropriate. We do not have to test node being able to resolve and load an installed package, we just have to test if the errors are handled. So mocking require and throwing the corresponding error as node would do should suffice in my opinion.

If no core-js installation can be found, an appropriate error should still be thrown.

ahnpnl · 2019-03-22T14:22:26Z

Maybe in your catch you can put sth to stdout to inform user about the version of core-js and in your test maybe you can capture that stdout ?

oktav777 · 2019-03-25T15:00:15Z

Well, what looked from the first sight "a hour of work" transformed in days.

I don't know why, but I can't make the tests work when I add conditional require(). I applied all of the changes into my project but test suites just hang on, they don't throw errors, no timeouts. Just nothing happens.

What I tried

dumping everything in one try/catch

try {
    require('core-js/es6/reflect');
    require('core-js/es/reflect');
    require('core-js/es7/reflect');
    require('core-js/proposals/reflect-metadata');
} catch (e) { }

separate try/catch for each version of Reflect (ES6/ES7)

try {
    require('core-js/es6/reflect');
} catch (e) {
    require('core-js/es/reflect');
}

try {
    require('core-js/es7/reflect');
} catch (e) {
    require('core-js/proposals/reflect-metadata');
}

fs.existsSync() with switch statement

var fs = require('fs');

const coreJS_2_es6reflect = 'core-js/es6/reflect';
const coreJS_3_es6reflect = 'core-js/es/reflect';

const coreJS_2_es7reflect = 'core-js/es7/reflect';
const coreJS_3_es7reflect = 'core-js/proposals/reflect-metadata';

switch(true) {
  case fs.existsSync(coreJS_2_es6reflect):
    require(coreJS_2_es6reflect);
    break;

  case fs.existsSync(coreJS_3_es6reflect):
    require(coreJS_3_es6reflect);
    break;

  default:
    console.log('ES6 `core-js/reflect` not found.');
    break;
}

switch(true) {
  case fs.existsSync(coreJS_2_es7reflect):
    require(coreJS_2_es7reflect);
    break;

  case fs.existsSync(coreJS_3_es7reflect):
    require(coreJS_3_es7reflect)
    break;

  default:
    console.log('ES7 `core-js/reflect` not found.');
    break;
}

using try-require package

var tryRequire = require('try-require');

if (tryRequire.resolve('core-js/es6/reflect')) {
    tryRequire('core-js/es6/reflect')
} else {
    tryRequire('core-js/es/reflect')
}

if (tryRequire.resolve('core-js/es7/reflect')) {
    tryRequire('core-js/es7/reflect')
} else {
    tryRequire('core-js/proposals/reflect-metadata')
}

None of the methods worked for me. I even started to doubt my coding skills...

I think maybe I missed something, and I would like to ask somebody to apply the changes I made to see if they will have the same result or not.

Also read the first paragraph from try-require#Usage about try/catch, require() function and v8 optimization.

Given that setupJest.ts runs for every test suite it may have an impact on performance.
More investigation needed.

wtho · 2019-03-27T22:30:18Z

I tried the second approach (try/catch) and it worked for me.

Note that I run rm -r node_modules && yarn install after each change to make sure the preset updated. Whatever you do in the parent folder is not copied automatically into the node_modules folder of the example project.

wtho · 2019-03-31T22:06:00Z

I also realized, that npm install creates symlinks, while yarn install creates a proper copy of the parent folder. I am not sure if both work the intended way.

I usually use yarn when working on jest-preset-angular, so does our CI, I recommend you to do the same.

oktav777 · 2019-04-01T08:06:27Z

I still can't handle the unit testing.

Using mock-require does not help.
A simple example:

let requireMock = require('mock-require');

it('should import from core-js@2', () => {
  const path = 'core-js/es6/reflect';

  requireMock(path, () => {
    console.log(`importing ${path}`);
    return true;
  });

  const module = require(path); 

  expect(module).toBeDefined();
});

And I get Cannot find module 'core-js/es6/reflect' from 'CoreJS.test.ts'

wtho · 2019-04-01T09:16:25Z

Yeah it's actually tricky.

I tried using jest.mock, but it only works if the mocked package is actually installed (for features like requireActual).

This means, we have to install @angular/(core|platform-browser-dynamic), zone.js and core-js. The installed core-js should also have the files according to the v2 folder structure as well as the v3 folder structure, so we can mock whatever whenever we want.

This is a limit, as we

do not want to install them just to mock them
cannot install two versions of core-js at once, yarn/npm won't let us

cc @thymikee do you think we should skip test this for now?

One idea I can come up with right now is to just add a smoke test in .circleci/config.yaml, where we run the example app with the installed core-js@v2 as is, then install core-js@v3 and then run at least one test again.

Edit:
Another idea, a dirty one, is to install these packages referencing a file "@angular/core": "file:__tests__/__mocks__/@angular/core" in devDependencies. This way we can provide everything we will mock in the tests.
The downside is that it makes the project structure quite confusing to contributers who are not familiar with this setup.

thymikee · 2019-04-01T10:28:16Z

You can use jest.mock('module', impl, {virtual: true}) for cases when module is not on the fs.

oktav777 · 2019-04-01T12:32:03Z

Do the module has to exist in __mocks__ folder?
When I use jest.mock(path, () => true, { virtual: true }), I get the error: TypeError: Cannot read property '0' of undefined

      72 |     const path = 'core-js/es/reflect';
      73 | 
    > 74 |     jest.mock(path, () => true, { virtual: true });
         |          ^
      75 | 
      76 |     const module = require(path);
      77 |

thymikee · 2019-04-01T12:59:13Z

Nothing needs to exist in __mocks__ folder for virtual mock to work. Can you share more of this error (where it comes from)? Maybe it's something that zone.js breaks.

wtho · 2019-04-01T15:10:41Z

Yeah, I did not use {virtual: true} or add __mocks__, that's why it failed before. I am still not 100% comfortable with jest mocks 😅

It works now with this setup:

describe('core-js loading', () => {
  beforeEach(() => {
    jest.resetAllMocks()

    jest.mock('@angular/core/testing', () => ({ getTestBed: () => ({initTestEnvironment() {}})}), {virtual: true})
    jest.mock('@angular/platform-browser-dynamic/testing', () => ({ BrowserDynamicTestingModule: null, platformBrowserDynamicTesting: () => {}}), {virtual: true})
    jest.mock('zone.js/dist/zone.js', () => {}, {virtual: true});	
    jest.mock('zone.js/dist/proxy.js', () => {}, {virtual: true});	
    jest.mock('zone.js/dist/sync-test', () => {}, {virtual: true});	
    jest.mock('zone.js/dist/async-test', () => {}, {virtual: true});	
    jest.mock('zone.js/dist/fake-async-test', () => {}, {virtual: true});	
    jest.mock('../zone-patch', () => {}, {virtual: true});	
  })

  it('should fail if no core-js is installed', () => {
    jest.mock('core-js/es6/reflect', () => {throw new Error('no v2 es6 reflect')}, {virtual: true});	
    jest.mock('core-js/es7/reflect', () => {throw new Error('no v2 es6 reflect')}, {virtual: true});	
    jest.mock('core-js/es/reflect', () => {throw new Error('no v3 es reflect')}, {virtual: true});	
    jest.mock('core-js/proposal/reflect-metadata', () => {throw new Error('no v3 proposal reflect-metadata')}, {virtual: true});	
    expect(() => require('../setupJest')).toThrow()
  })

  it('should pass if core-js v2 is installed', () => {
    jest.mock('core-js/es6/reflect', () => {}, {virtual: true});
    jest.mock('core-js/es7/reflect', () => {}, {virtual: true});
    jest.mock('core-js/es/reflect', () => {throw new Error('no v3 es reflect')}, {virtual: true});	
    jest.mock('core-js/proposal/reflect-metadata', () => {throw new Error('no v3 proposal reflect-metadata')}, {virtual: true});	
    expect(() => require('../setupJest')).toThrow()
  })
})

My setupJest

let coreJsLoadded = false
try {
  require('core-js/es6/reflect');	
  require('core-js/es7/reflect');	
  coreJsLoadded = true
} catch (_) {}
try {
  require('core-js/es/reflect');	
  require('core-js/proposals/reflect-metadata');	
  coreJsLoadded = true
} catch (_) {}
if (!coreJsLoadded) {
  throw new Error('Could not load module core-js, make sure it is installed')
}

require('...');
...

oktav777 · 2019-04-02T07:02:44Z

I'm kinda new to Jest too 😄
Thank you @wtho, that made things more clear for me. I will try to find some time today to update the PR.

dmitriivikorchuk · 2019-04-04T10:10:59Z

Does this not help you?
switch to require('core-js/feature/reflect')

oktav777 · 2019-04-08T13:25:21Z

@thymikee, I added the tests and squashed + rebased the PR.
Take a look.

thymikee

LGTM, @wtho mind having a final look?

wtho

Just this one naming might be chosen better, otherwise it looks awesome!

wtho · 2019-04-08T14:51:12Z

__tests__/CoreJS-es6-reflect.test.ts

@@ -0,0 +1,44 @@
+describe('importing ES6 reflect', () => {
+
+  let resetMocks = () => {


better call this setMocks

will update tomorrow

thymikee · 2019-04-09T06:52:25Z

Thank you so much @oktav777!

thymikee · 2019-04-09T06:54:24Z

Released in 7.1.0

thymikee reviewed Mar 22, 2019

View reviewed changes

oktav777 force-pushed the master branch from 1f2ad1a to 98a68e4 Compare April 8, 2019 13:20

thymikee approved these changes Apr 8, 2019

View reviewed changes

wtho approved these changes Apr 8, 2019

View reviewed changes

feat: support core-js >=2.0.0 < 4.0.0

0dea349

oktav777 force-pushed the master branch from 98a68e4 to 0dea349 Compare April 9, 2019 06:46

thymikee merged commit 239a647 into thymikee:master Apr 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(package): update core-js to version 3.0.0 #246

fix(package): update core-js to version 3.0.0 #246

oktav777 commented Mar 22, 2019 •

edited

Loading

thymikee Mar 22, 2019

oktav777 Mar 22, 2019

thymikee Mar 22, 2019

oktav777 Mar 22, 2019

thymikee Mar 22, 2019

wtho Mar 23, 2019

ahnpnl commented Mar 22, 2019

oktav777 commented Mar 25, 2019

wtho commented Mar 27, 2019 •

edited

Loading

wtho commented Mar 31, 2019 •

edited

Loading

oktav777 commented Apr 1, 2019 •

edited

Loading

wtho commented Apr 1, 2019 •

edited

Loading

thymikee commented Apr 1, 2019

oktav777 commented Apr 1, 2019

thymikee commented Apr 1, 2019

wtho commented Apr 1, 2019 •

edited

Loading

oktav777 commented Apr 2, 2019

dmitriivikorchuk commented Apr 4, 2019

oktav777 commented Apr 8, 2019

thymikee left a comment

wtho left a comment

wtho Apr 8, 2019

oktav777 Apr 8, 2019

oktav777 Apr 9, 2019

thymikee commented Apr 9, 2019

thymikee commented Apr 9, 2019

		@@ -0,0 +1,44 @@
		describe('importing ES6 reflect', () => {

		let resetMocks = () => {

fix(package): update core-js to version 3.0.0 #246

fix(package): update core-js to version 3.0.0 #246

Conversation

oktav777 commented Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnpnl commented Mar 22, 2019

oktav777 commented Mar 25, 2019

wtho commented Mar 27, 2019 • edited Loading

wtho commented Mar 31, 2019 • edited Loading

oktav777 commented Apr 1, 2019 • edited Loading

wtho commented Apr 1, 2019 • edited Loading

thymikee commented Apr 1, 2019

oktav777 commented Apr 1, 2019

thymikee commented Apr 1, 2019

wtho commented Apr 1, 2019 • edited Loading

oktav777 commented Apr 2, 2019

dmitriivikorchuk commented Apr 4, 2019

oktav777 commented Apr 8, 2019

thymikee left a comment

Choose a reason for hiding this comment

wtho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee commented Apr 9, 2019

thymikee commented Apr 9, 2019

oktav777 commented Mar 22, 2019 •

edited

Loading

wtho commented Mar 27, 2019 •

edited

Loading

wtho commented Mar 31, 2019 •

edited

Loading

oktav777 commented Apr 1, 2019 •

edited

Loading

wtho commented Apr 1, 2019 •

edited

Loading

wtho commented Apr 1, 2019 •

edited

Loading