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

Use Object.assign #630

Merged
merged 1 commit into from
Jul 28, 2022
Merged

Use Object.assign #630

merged 1 commit into from
Jul 28, 2022

Conversation

tschaub
Copy link
Member

@tschaub tschaub commented Jul 27, 2022

This makes use of Object.assign instead of importing assign from the ol/obj.js module.

See openlayers/openlayers#13888.

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thanks!

@ahocevar
Copy link
Member

Feel free to merge even without the check run, I either need to switch to GitHub Actions or figure out why CircleCI does not pick up your pull request.

@ahocevar
Copy link
Member

I took the liberty to rebase your branch, because I have now configured GitHub Actions. Looks like the previously expected circleci check cannot be removed from existing pull requests, but with the Node.js CI check green this should be safe to merge.

@tschaub tschaub merged commit d921f19 into openlayers:main Jul 28, 2022
@tschaub tschaub deleted the object-assign branch July 28, 2022 10:30
@tschaub
Copy link
Member Author

tschaub commented Jul 28, 2022

I removed the CircleCI job from the required status checks under the branch protection rules. And I added the new build (16.x) to the required status checks.

image

A couple only somewhat related things that you may or may not want to consider. The names for the required status checks come from a concatenation of the job name in the yaml file and version from the strategy matrix.

So in this case, the status check will be prefixed with build:

If you wanted it to be named test for example, that line would change. I only mention this because I always get it mixed up with the name at the top of the workflow file - never knowing which is going to be the name in the required status check.

If you are only really concerned about testing against one version of Node, I think it is nicer to remove the strategy matrix and just use a fixed Node version.

--- a/.github/workflows/node.js.yml
+++ b/.github/workflows/node.js.yml
@@ -14,17 +14,11 @@ jobs:
 
     runs-on: ubuntu-latest
 
-    strategy:
-      matrix:
-        node-version: [16.x]
-        # See supported Node.js release schedule at https://nodejs.org/en/about/releases/
-
     steps:
     - uses: actions/checkout@v3
-    - name: Use Node.js ${{ matrix.node-version }}
-      uses: actions/setup-node@v3
+    - uses: actions/setup-node@v3
       with:
-        node-version: ${{ matrix.node-version }}
+        node-version: '16.x'
         cache: 'npm'
     - run: npm ci
     - run: npm run build --if-present

That way, the required status check name is fixed (build in this case) and you don't have to edit your branch protection rules any time you update the Node version (which is a hassle because you can't add the new status check as required until it has been run).

See #634 .

@ahocevar
Copy link
Member

Thanks, @tschaub.

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.

None yet

2 participants