Skip to content

Conversation

eiri
Copy link
Contributor

@eiri eiri commented Aug 8, 2024

PR summary

This is a build refactoring work that makes sure we are using esm style imports in all of our custom modules.

PR Checklist

Please make sure that your PR fulfills the following requirements:

  • The commit message follows the
    Angular Commit Message Guidelines.
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • New tests
  • Build/CI related changes
  • Documentation content changes
  • Other (please describe)

What is the current behavior?

We have a mix of import and required statements.

What is the new behavior?

We still have required in generated modules, but our custom modules are now have consistent imports.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@eiri eiri force-pushed the s870-consistent-imports branch 2 times, most recently from e177024 to 6918612 Compare August 13, 2024 11:40
@eiri eiri marked this pull request as ready for review August 13, 2024 11:43
@eiri eiri requested a review from a team as a code owner August 13, 2024 11:43
@eiri eiri force-pushed the s870-consistent-imports branch from dcd2221 to 206ab65 Compare August 13, 2024 13:15
@mojito317 mojito317 self-requested a review August 13, 2024 14:19
Copy link
Contributor

@mojito317 mojito317 left a comment

Choose a reason for hiding this comment

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

A couple of questions and change suggestions.

@eiri eiri force-pushed the s870-consistent-imports branch from 206ab65 to 753c58e Compare September 17, 2024 19:32
* See the License for the specific language governing permissions and
* limitations under the License.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing extension of this file to .mjs will need changes in README rendering as well.
This comment is also relevant to the other files in this directory.

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, but that's in templates and since README's already resolved and examples embedded I expect this PR doesn't break it, just needs a followup in templates in upstream.

@eiri
Copy link
Contributor Author

eiri commented Sep 18, 2024

@mojito317 it turned out jest was ignoring mjs tests and once I configured it not to it became really hard to make mix of cjs and esm import styles to work together in tests. So this PR superseded by #1629 that uses imports in lib and consistent required in tests. Sorry about the wasted review.

@eiri eiri closed this Sep 18, 2024
@eiri eiri deleted the s870-consistent-imports branch September 18, 2024 17:15
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.

2 participants