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

feat(adapter): update createConfig helper to use deep merge #38

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

tanner-reits
Copy link
Contributor

@tanner-reits tanner-reits commented Apr 12, 2024

What is the current behavior?

With the current API, the process of overriding nested object values in the config is tedious. A user either has to use custom override options we create on a type (like webServerCommand), or use a .then() on the helper to update specific values on the generated config.

GitHub Issue Number: N/A

What is the new behavior?

The helper will now use a "deep merge" approach for overrides so only the specified properties in an object get changed, not the whole object. So, a call to the helper like:

import { expect } from '@playwright/test';
import { matchers, createConfig } from '@stencil/playwright';

expect.extend(matchers);

export default createConfig({
  // Change which test files Playwright will execute
  testMatch: '*.spec.ts',

  webServer: {
    // Only wait max 30 seconds for server to start
    timeout: 30000,
  },
});

Will generate the following config:

{
  testMatch: '*.spec.ts',
  use: {
    baseURL: 'http://localhost:3333',
  },
  webServer: {
    command: 'stencil build --dev --watch --serve --no-open',
    url: 'http://localhost:3333/ping',
    reuseExistingServer: !process.env.CI,
    // Only timeout gets overridden, not the entire object
    timeout: 30000,
  },
}

Documentation

stenciljs/site#1399

Does this introduce a breaking change?

  • Yes
  • No

Technically a breaking change, but this is a pre-1.0 experimental package.

Testing

Updated automated tests and confirmed the functionality in a Stencil starter project

Other information

@tanner-reits tanner-reits marked this pull request as ready for review April 12, 2024 15:35
@tanner-reits tanner-reits requested a review from a team as a code owner April 12, 2024 15:35
@tanner-reits tanner-reits requested review from christian-bromann and alicewriteswrongs and removed request for a team April 12, 2024 15:35
Copy link

@alicewriteswrongs alicewriteswrongs left a comment

Choose a reason for hiding this comment

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

I think this is a good and ergonomic change that makes our stuff 'closer' to playwright! I support it. I noticed one thing I think we'll want to adjust with the typing though

Copy link

@alicewriteswrongs alicewriteswrongs left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

One optional suggestion. Given the small impact of this dependency I don't think it will be a problem but still worth to consider.

@tanner-reits tanner-reits force-pushed the tr/create-config-api branch from 9f0c091 to 2893d4b Compare April 15, 2024 19:57
Base automatically changed from tr/rename-create-config-helper to main April 16, 2024 15:02
@tanner-reits tanner-reits force-pushed the tr/create-config-api branch from 2893d4b to 9b2f8b5 Compare April 16, 2024 15:07
@tanner-reits tanner-reits added this pull request to the merge queue Apr 16, 2024
Merged via the queue into main with commit 6a4c0bb Apr 16, 2024
3 checks passed
@tanner-reits tanner-reits deleted the tr/create-config-api branch April 16, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants