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

DS-409 Update plop component generator #2292

Merged
merged 24 commits into from
Sep 17, 2021

Conversation

MarcinMr
Copy link
Collaborator

@MarcinMr MarcinMr commented Aug 5, 2021

Jira

https://pegadigitalit.atlassian.net/browse/DS-409

Summary

Schema, twig and test templates were updated to our standards

Details

A branch feature/DS-408-Convert-Yeoman-to-Plop-alternative was pulled to this one.

  • A new prompt was added to the component generator that will render a different JS, Jest Test and Twig file if this component is a web component.
  • title and content were added to component.schema
  • component.docs.twig was updated with single quotes
  • component.twig was updated with single quotes, attributes was changed with props.class, title and content were added to the template
  • component.test.js was updated with commented code in case to use, according to the master schema
  • element.docs.twig was updated with single quotes

How to test

  1. Pull the branch locally and run yarn setup.
  2. Run yarn cc to generate component and check if all the files are generated correctly (remember to run yarn after).
  3. On the "Is this component a web component?:" press enter ("n" is the default value)
  4. Run yarn cc to generate component and check if all the files are generated correctly (remember to run yarn after).
  5. On the "Is this component a web component?:" question type "y"
  6. Run yarn ce to generate element and check if all the files are generated correctly (remember to run yarn after).

@github-actions github-actions bot added the type: feature List this PR in the 'Features' section of the release notes. label Aug 5, 2021
@colbytcook colbytcook temporarily deployed to feature/DS-409-update-plop-component-generator--branch-preview August 5, 2021 12:42 Inactive
…ator

# Conflicts:
#	packages/generators/yeoman-create-component/CHANGELOG.md
#	packages/generators/yeoman-create-component/package.json
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @MarcinMr. I just made one formatting fix.

@colbytcook colbytcook temporarily deployed to feature/DS-409-update-plop-component-generator--branch-preview August 9, 2021 18:18 Inactive
@colbytcook colbytcook temporarily deployed to feature/DS-409-update-plop-component-generator--branch-preview August 11, 2021 20:21 Inactive
@colbytcook colbytcook temporarily deployed to feature/DS-409-update-plop-component-generator--branch-preview August 12, 2021 14:40 Inactive
Copy link
Collaborator

@cjwhitedev cjwhitedev left a comment

Choose a reason for hiding this comment

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

  1. if we're trying to make less and less web components, do we want the default generator to be a web component out of the box? maybe we can add a step in the generator to ask if its a web component, then generate a separate directory.
  2. in the twig file it doesnt look to me like we are properly leveraging this.data / validation out of the box.
  3. in the test file, are the right things commented out? if we are creating a web component by default, we should be testing a web component by default. if we go with any changes from my first comment above, then the test file should be rendered differently as well, right?
    ---- editing in a little more ----
  4. should we generate the first docs file too? so we have an example of what we want the docs to look like from here out. this could use some input from @mikemai2awesome since he's been rocking the documentation.

@mikemai2awesome
Copy link
Collaborator

yes, we should chat about CJ's points.

Copy link
Collaborator

@adamszalapski adamszalapski left a comment

Choose a reason for hiding this comment

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

Why did we change the name of the package? If this was already. discuss we should find all bolt-generator occurrences and change it to generator-bolt because of other tests and some exceptions based on this name.

@colbytcook colbytcook temporarily deployed to feature/DS-409-update-plop-component-generator--branch-preview September 7, 2021 15:22 Inactive
@colbytcook colbytcook had a problem deploying to feature/DS-409-update-plop-component-generator--branch-preview September 13, 2021 18:44 Failure
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

This looks good except for the tests. I'm running into some Jest test errors locally that I'll need more time to debug. Tests pass individually, and then when I run yarn test:js several generator tests fail.

Also, made a very minor fix. We were missing a closing single-quote in the commented-out test, and we included an extra space at the end. Jest adds a space for you, no need for a trailing space.

@colbytcook colbytcook requested a deployment to feature/DS-483-code-snippet-updates--74206cfb--commit-preview September 14, 2021 22:06 In progress
Copy link
Collaborator

@adamszalapski adamszalapski left a comment

Choose a reason for hiding this comment

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

Looks good. I agree with @danielamorse all-new approaches/conventions should be updated/added to our WIKI.

@colbytcook colbytcook temporarily deployed to feature/DS-409-update-plop-component-generator--branch-preview September 15, 2021 15:04 Inactive
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

I'm seeing Jest test failures when I do the following:

  1. Run yarn test:js (all tests pass... although I was seeing unrelated intermittent bolt-text fails, disregard for now)
  2. Run npx jest packages/generators/bolt-generator/__tests__/component/bolt-generator.js (pass)
  3. Run npx jest packages/generators/bolt-generator/__tests__/element/bolt-generator.js (pass)
  4. Run yarn test:js (fail, see error)

The second time you run yarn test:js it produces different snapshots. It seems like something is left over from previous tests (perhaps in tmp?) that changes the test results.

I discovered this when I had to update the individual generator snapshots. Then I ran all tests and saw the error. It's an edge case, but we should fix it before this gets merged.

@adamszalapski any thoughts on why this might happen?

@adamszalapski
Copy link
Collaborator

adamszalapski commented Sep 16, 2021

Good catch @danielamorse

This issue's related to testing asynchronously and how our bolt-generator uses tmp folder for checking the file structure. When we run yarn test:js our tests are run asynchronously and in some cases, tests for component and element can be run at the same time.

To prevent this situation I updated the approach how we use tmp folder and now the issue is gone. This was a tricky one to catch, good work @danielamorse 👍

@colbytcook colbytcook temporarily deployed to feature/DS-409-update-plop-component-generator--branch-preview September 16, 2021 14:41 Inactive
@adamszalapski
Copy link
Collaborator

adamszalapski commented Sep 17, 2021

@danielamorse I update whole folder structure for bolt-generator tests, right now temp folders are generated in specific (component or element) folder. At the moment all test passing without any issue, please double check if everything is good.

image

@colbytcook colbytcook requested a deployment to feature/DS-409-update-plop-component-generator--branch-preview September 17, 2021 10:54 Abandoned
Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

@adamszalapski works perfectly now. Thank you!

@danielamorse danielamorse merged commit 9dfc62c into master Sep 17, 2021
@danielamorse danielamorse deleted the feature/DS-409-update-plop-component-generator branch September 17, 2021 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature List this PR in the 'Features' section of the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants