Skip to content

feat(@angular-devkit/build-angular): differential loading check #13993

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

Closed
wants to merge 1 commit into from
Closed

feat(@angular-devkit/build-angular): differential loading check #13993

wants to merge 1 commit into from

Conversation

manfredsteyer
Copy link
Contributor

No description provided.

Copy link
Member

@mgechev mgechev left a comment

Choose a reason for hiding this comment

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

Exciting to see this getting started :)

@alan-agius4

This comment has been minimized.

@manfredsteyer manfredsteyer requested a review from clydin March 28, 2019 17:31
@@ -0,0 +1,75 @@
/**
Copy link
Collaborator

@alan-agius4 alan-agius4 Apr 1, 2019

Choose a reason for hiding this comment

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

I think there is a bit of misunderstanding here, specs are usefully next to the feature file. These specs in in this folder are usefully used to test the architect and named large.

These are more of an integration test rather than a unit. Unit tests should reside next to the feature.

Also, we should not interact with the File System directly, we should use the Abstraction layer as this would reduce a couple of things.

  1. Makes mocking easier.
  2. No need to create a physical file for each test.
  3. Have the mocks as part of the spec file hence makes the test more readable.
const workspaceRoot = path.join(
  devkitRoot,
  'tests/angular_devkit/build_angular/test-differential-loading/');
const host = new TestProjectHost(normalize(workspaceRoot));

describe('differential loading', () => {
  it('detects the need for ES5 if IE 9-11 has to be supported', () => {
    const path = normalize('/browerlist');
    host.writeMultipleFiles({
        path: 'last 1 chrome version'
    });
    const es5SupportNeeded = isEs5BrowserSupportNeeded(workspaceRoot, path);
    expect(es5SupportNeeded).toBe(true);
  });
});

The host itself has a lot for method, that can help you with transforming a file, overriding it's content, delete it etc..., the important one is that it will never override the file on the hard drive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Umm actually TestProjectHost seems to be deprecated, @clydin what's the new preferred way to approach this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still pending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried this. While this totally makes sense, it seems that in this very case, using the virtual filesystem does not work. The reason is that browserslist has its own machanism for finding and loading the browswerslist config. This one isn't using the virtual filesystem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should work, as the TestProjectHost writes files to disks.

See below;
Screenshot 2019-04-02 at 17 45 13

If you need a hand let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be a Windows-related issue: host.root() returns sth like /c/my-dir instead of c:/my-dir. The forward slashes are no issue here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that is expected if you pass a devkit path directly to the method yourself that is using FS.

Devkit paths are always posix normalised, which means on windows c:\\ will be /c/ which means that any interaction with FS will fail on windows. Devkit paths/methods cannot be used with direct interaction with fs.

The projectRoot, is normalised is passed from wco. See https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/browser/index.ts#L127

The safest and cleaner approach would be changing the projectRoot to Path and using getSystemPath

export function isDifferentialLoadingNeeded(
  projectRoot: Path,
  target: ts.ScriptTarget): boolean {

   const supportES2015 = target !== ts.ScriptTarget.ES3 && target !== ts.ScriptTarget.ES5;
  const browsersList: string[] = browserslist(
    undefined, {
      path: getSystemPath(projectRoot),
    });

   return supportES2015 && !caniuse.isSupported('es6-module', browsersList.join(', '));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Comments above.

@alan-agius4
Copy link
Collaborator

@manfredsteyer is this absolute in favor of the other PR or not?

@manfredsteyer
Copy link
Contributor Author

manfredsteyer commented Apr 5, 2019

@manfredsteyer is this absolute in favor of the other PR or not?

Yes, it is needed by the other one, @alan-agius4

This checks if differential loading is needed by looking at the target version and by executing the browserslist. If the browserslist points to ES5 browsers and the target version points to ES2015+, differential loading is needed.

https://docs.google.com/document/d/13k84oGwrEjwPyAiAjUgaaM7YHJrzYXz7Cbt6CwRp9N4/edit?ts=5c652052
@alan-agius4 alan-agius4 closed this Apr 9, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants