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

Converted dev scripts to ESM modules #720

Merged
merged 9 commits into from
Oct 22, 2020

Conversation

KolCrooks
Copy link
Contributor

I converted the dev scripts to ESM modules as talked about in #701.

@CLAassistant
Copy link

CLAassistant commented Oct 8, 2020

CLA assistant check
All committers have signed the CLA.

@KolCrooks KolCrooks mentioned this pull request Oct 8, 2020
9 tasks
@MarcusNotheis MarcusNotheis linked an issue Oct 9, 2020 that may be closed by this pull request
9 tasks
@MarcusNotheis
Copy link
Contributor

MarcusNotheis commented Oct 15, 2020

Hey @KolCrooks,
could you update your branch with the latest master version (or give me write access to that branch in case there are more commits that have to be merged)? Then we can get this into our codebase.
Thanks!

EDIT: sorry, just realized that I already have write access

Copy link
Contributor

@MarcusNotheis MarcusNotheis left a comment

Choose a reason for hiding this comment

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

It looks like jest serializers have to be in cjs format, so could you please revert the changes in both serializers (marked as PR comment) and rename them to use the .cjs file extension?
Then you have to update the imports in config/jestsetup.ts to match the new .cjs file extension:

import contentLoaderSerializer from '@shared/tests/serializer/content-loader-serializer.cjs';
import jssSerializer from '@shared/tests/serializer/jss-snapshot-serializer.cjs';

Other than that everything looks good and we can merge this PR as soon as all CI jobs are passing.

Thanks!

shared/tests/serializer/content-loader-serializer.js Outdated Show resolved Hide resolved
shared/tests/serializer/jss-snapshot-serializer.js Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage remained the same at 65.448% when pulling 6f74bfd on KolCrooks:ESMDevScripts into 45a6d8f on SAP:master.

@MarcusNotheis MarcusNotheis merged commit 18482a1 into SAP:master Oct 22, 2020
@KolCrooks
Copy link
Contributor Author

@MarcusNotheis I'm sorry that I haven't been able to work on this / not giving a heads up. I got hit with a ton of work and midterms from uni so It has been tough for me to find time to do things like this.

@MarcusNotheis
Copy link
Contributor

Key @KolCrooks,
No worries, all good - as it was only a very little change that was pending I just added and merged it :) thanks for your work on that PR and good luck for your midterms :)

Best regards,
Marcus

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.

use ESM Modules for dev scripts
4 participants