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

New Custom Views Javascript template #3295

Merged
merged 30 commits into from
Nov 20, 2023
Merged

Conversation

CarlosCortizasCT
Copy link
Contributor

Summary

New Custom Views Javascript template

Description

So far we only had a Typescript version of the template to bootstrap a new Custom View project.
We're adding a new Javascript based template.

Copy link

changeset-bot bot commented Nov 13, 2023

🦋 Changeset detected

Latest commit: 30f01ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 38 packages
Name Type
@commercetools-applications/merchant-center-custom-view-template-starter Minor
@commercetools-frontend/create-mc-app Minor
@commercetools-applications/merchant-center-template-starter-typescript Minor
@commercetools-applications/merchant-center-template-starter Minor
@commercetools-applications/merchant-center-custom-view-template-starter-typescript Minor
@commercetools-backend/eslint-config-node Minor
@commercetools-backend/express Minor
@commercetools-backend/loggers Minor
@commercetools-frontend/actions-global Minor
@commercetools-frontend/application-components Minor
@commercetools-frontend/application-config Minor
@commercetools-frontend/application-shell-connectors Minor
@commercetools-frontend/application-shell Minor
@commercetools-frontend/assets Minor
@commercetools-frontend/babel-preset-mc-app Minor
@commercetools-frontend/browser-history Minor
@commercetools-frontend/codemod Minor
@commercetools-frontend/constants Minor
@commercetools-frontend/cypress Minor
@commercetools-frontend/eslint-config-mc-app Minor
@commercetools-frontend/i18n Minor
@commercetools-frontend/jest-preset-mc-app Minor
@commercetools-frontend/jest-stylelint-runner Minor
@commercetools-frontend/l10n Minor
@commercetools-frontend/mc-dev-authentication Minor
@commercetools-frontend/mc-html-template Minor
@commercetools-frontend/mc-scripts Minor
@commercetools-frontend/notifications Minor
@commercetools-frontend/permissions Minor
@commercetools-frontend/react-notifications Minor
@commercetools-frontend/sdk Minor
@commercetools-frontend/sentry Minor
@commercetools-frontend/url-utils Minor
@commercetools-local/playground Minor
@commercetools-local/visual-testing-app Minor
@commercetools-website/custom-applications Minor
@commercetools-website/components-playground Minor
@commercetools-website/custom-views Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@CarlosCortizasCT
Copy link
Contributor Author

@emmenko do you think we can already remove the experimental flag for creating Custom Views templates or should we keep it until the General Available stage?

const throwIfApplicationTypeIsNotSupported = (
  applicationType: TApplicationType
) => {
  switch (applicationType) {
    case applicationTypes['custom-view']: {
      if (process.env.ENABLE_EXPERIMENTAL_CUSTOM_VIEWS !== 'true') {
        throw new Error(`Custom Views generation is not yet supported.`);
      }
      break;
    }
    case applicationTypes['custom-application']:
      break;
    default: {
      const applicationTypesList = Object.keys(applicationTypes).toString();
      throw new Error(
        `The provided application type "${applicationType}" does not exist. Available types are "${applicationTypesList}". Make sure you are also using the latest version of "@commercetools-frontend/create-mc-app".`
      );
    }
  }
};

Copy link
Contributor

github-actions bot commented Nov 13, 2023

Deploy preview for merchant-center-application-kit ready!

✅ Preview
https://merchant-center-application-7fmcnnor4-commercetools.vercel.app
https://appkit-sha-792076bc33d945ad18d15d2dcff1c3522971ed3f.commercetools.vercel.app
https://appkit-pr-3295.commercetools.vercel.app

Built with commit 30f01ac.
This pull request is being automatically deployed with vercel-action

Copy link
Contributor

github-actions bot commented Nov 13, 2023

Deploy preview for application-kit-custom-views ready!

✅ Preview
https://application-kit-custom-views-azejoax10-commercetools.vercel.app
https://appkit-cv-sha-792076bc33d945ad18d15d2dcff1c3522971ed3f.commercetools.vercel.app
https://appkit-cv-pr-3295.commercetools.vercel.app

Built with commit 30f01ac.
This pull request is being automatically deployed with vercel-action

@CarlosCortizasCT CarlosCortizasCT requested a review from a team November 13, 2023 11:55
@CarlosCortizasCT
Copy link
Contributor Author

@emmenko I also noticed we're testing Custom Applications template in CI but we're not doing it for Custom Views. Should I update the configuration to enable it?

@emmenko
Copy link
Member

emmenko commented Nov 14, 2023

@emmenko I also noticed we're testing Custom Applications template in CI but we're not doing it for Custom Views. Should I update the configuration to enable it?

Yes please, that was a leftover I think

@emmenko
Copy link
Member

emmenko commented Nov 14, 2023

@emmenko do you think we can already remove the experimental flag for creating Custom Views templates or should we keep it until the General Available stage?

I think we can keep it until general availability considering that the docs won't be "publicly" visible yet and that the configuration UI is also enabled for early adopters.

@CarlosCortizasCT
Copy link
Contributor Author

I think we can keep it until general availability considering that the docs won't be "publicly" visible yet and that the configuration UI is also enabled for early adopters.

Sorry I didn't give all the context in my first question.

I was thinking that now we will be having users starting to develop Custom Views, it would be easier for them if we don't ask them to set the env var.

If we keep the experimental flag, we need to also update the (private) docs to include over there the env variable consumers need to set in order for the cli to work.

Would it be ok to just update the example command?

shell> ENABLE_EXPERIMENTAL_CUSTOM_VIEWS="true" npx @commercetools-frontend/create-mc-app@latest \
  my-new-custom-view-project \
  --application-type custom-view \
  --template starter

@emmenko
Copy link
Member

emmenko commented Nov 16, 2023

I was thinking that now we will be having users starting to develop Custom Views, it would be easier for them if we don't ask them to set the env var.

Good point. Ok then if you think it would be better we can remove the flag. If people ask and require access to Custom Views we can say it's in private beta and add them if they want to.

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Awesome job! 🙌

Great idea with the composite action to simplify the setup and to have a consistent commands 🎉

run: pnpm template-starter-typescript:build
env:
CTP_INITIAL_PROJECT_KEY: ${{ secrets.CYPRESS_PROJECT_KEY }}
# TODO: Uncomment when we have Custom View e2e tests implemented
Copy link
Member

Choose a reason for hiding this comment

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

We can keep the step active and define a dummy command

"test:e2e:template-starter-custom-view": "echo \"No tests implemented yet\""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here: adc437d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I didn't understand the suggestion the first time I read it.

I updated after our meeting in this commit: b89ac73

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this file is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed here: 1b69e24

Copy link
Member

Choose a reason for hiding this comment

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

This is also not needed.

Did you copy the Custom Application JS template or the Custom View TS template (and removed the types)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed here: 1b69e24

I copied the Custom Application JS template.

package.json Outdated
Comment on lines 52 to 54
"template-custom-view-starter-javascript:build": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run build",
"template-custom-view-starter-javascript:start": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run start",
"template-custom-view-starter-javascript:start:prod:local": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run start:prod:local",
Copy link
Member

Choose a reason for hiding this comment

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

Nit: names seem to be a bit inconsistent with the Custom Applications

Suggested change
"template-custom-view-starter-javascript:build": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run build",
"template-custom-view-starter-javascript:start": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run start",
"template-custom-view-starter-javascript:start:prod:local": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run start:prod:local",
"template-custom-view-starter:build": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run build",
"template-custom-view-starter:start": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run start",
"template-custom-view-starter:start:prod:local": "pnpm --filter @commercetools-applications/merchant-center-custom-view-template-starter run start:prod:local",

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 agree they are inconsistent right now as I didn't want to change anything related to Custom Applications just in case there were any unexpected impact.

From my point of view, Custom Application commands are the ones who lack a more meaningful name and what I think it would have more sense is something like this:

"template-custom-application-starter-javascript:build"
"template-custom-application-starter-javascript:start"
"template-custom-application-starter-javascript:start:prod:local"
"template-custom-application-starter-typescript:build"
"template-custom-application-starter-typescript:start"
"template-custom-application-starter-typescript:start:prod:local"

"template-custom-view-starter-javascript:build"
"template-custom-view-starter-javascript:start"
"template-custom-view-starter-javascript:start:prod:local"
"template-custom-view-starter-typescript:build"
"template-custom-view-starter-typescript:start"
"template-custom-view-starter-typescript:start:prod:local"

Does it make sense to you our do you prefer to infer javascript is the default implementation?

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the clarification.

From my experience using other tools and libraries, JS is normally the default implementation yes. However, I do see your point and I don't mind either way. For me it's more important to have it consistent.

Maybe you can run a quick poll in the team to get their feedback as well. Renaming the Custom Applications template then should be a follow up.
Thanks

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 updated to default names to Javascript here: adc437d

"template-custom-application-starter:build"
"template-custom-application-starter:start"
"template-custom-application-starter:start:prod:local"
"template-custom-application-starter-typescript:build"
"template-custom-application-starter-typescript:start"
"template-custom-application-starter-typescript:start:prod:local"

"template-custom-view-starter:build"
"template-custom-view-starter:start"
"template-custom-view-starter:start:prod:local"
"template-custom-view-starter-typescript:build"
"template-custom-view-starter-typescript:start"
"template-custom-view-starter-typescript:start:prod:local"

Comment on lines +90 to +101
console.log('==> Assert application config file exists');
if (applicationType === 'custom-application') {
assert.strictEqual(
doesFileExist(path.join(applicationPath, 'custom-application-config.mjs')),
true
);
} else if (applicationType === 'custom-view') {
assert.strictEqual(
doesFileExist(path.join(applicationPath, 'custom-view-config.mjs')),
true
);
}
Copy link
Member

Choose a reason for hiding this comment

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

💯

Comment on lines +93 to +98
doesFileExist(path.join(applicationPath, 'custom-application-config.mjs')),
true
);
} else if (applicationType === 'custom-view') {
assert.strictEqual(
doesFileExist(path.join(applicationPath, 'custom-view-config.mjs')),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we can simply use existsSync (we use it many places in the repo), so there is no need to have a try/catch

Suggested change
doesFileExist(path.join(applicationPath, 'custom-application-config.mjs')),
true
);
} else if (applicationType === 'custom-view') {
assert.strictEqual(
doesFileExist(path.join(applicationPath, 'custom-view-config.mjs')),
fs.existsSync(path.join(applicationPath, 'custom-application-config.mjs')),
true
);
} else if (applicationType === 'custom-view') {
assert.strictEqual(
fs.existsSync(path.join(applicationPath, 'custom-view-config.mjs')),

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 used this implementation as it's the one recommended in Nodejs docs since fs.exists is a deprecated function.

image

Copy link
Member

Choose a reason for hiding this comment

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

🤦‍♂️ My bad, sorry.

I guess we would need to refactor the other places then to stop using fs.existsSync.
I'll create a task.

Comment on lines 36 to 37
# - name: Checkout
# uses: actions/checkout@v4
Copy link
Member

Choose a reason for hiding this comment

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

I guess this can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed here: adc437d


inputs:
template-name:
description: 'Name of the template to test'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this is currently used as e.g. template-starter-typescript or template-custom-view-starter-typescript

When I think about the create-mc-app CLI options, the --template option is either starter or starter-template.

I'm wondering if maybe we could have a similar properties here like application-type and template-name.

Example usage:

- name: Test Custom Application typescript starter template
  uses: ./.github/actions/test-template-action
  with:
    application-type: custom-application
    template-name: starter-typescript

With these two properties, we can then build the command names or whatever we need to. The main benefit I see with this is naming consistency and familiarity.

Just a suggestion anyway, happy to hear your thoughts 🤗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree it's easier to understand.

Updated here: adc437d

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Great work, thanks again 🙌 🚀

Copy link
Member

Choose a reason for hiding this comment

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

Nit: do we need a changeset for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here: d63c893

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