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

feat(dist-custom-elements-bundle): add deprecation warning #3167

Merged
merged 4 commits into from
Dec 6, 2021

Conversation

willmartian
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Unit tests (npm test) were run locally and passed
  • E2E Tests (npm run test.karma.prod) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

dist-custom-elements-bundle is being deprecated in an upcoming major release of Stencil. See discussion in the following GitHub issue. Currently, no warning is shown to the user communicating this.

GitHub Issue Number: #3136

What is the new behavior?

If dist-custom-elements-bundle is found in a Stencil config, a warning is fired when the build process begins that states: dist-custom-elements-bundle is deprecated and will be removed in a future version. Use "dist-custom-elements" instead. If "dist-custom-elements" does not meet your needs, please add a comment to https://github.com/ionic-team/stencil/issues/3136.

Does this introduce a breaking change?

  • Yes
  • No

Testing

A npm package tarball was generated from this branch (npm ci && npm run build && npm pack). It was then installed in a fresh Stencil project (npx init stencil). Said project was built both with and without dist-custom-elements-bundle being present in the output targets config to verify that the warning only appeared when the output target is present.

Other information

@willmartian willmartian requested a review from a team December 3, 2021 15:29
Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

Left a few comments/suggestions, LMK what you think.

@splitinfinities PTAL and verify this meets your acceptance criteria surrounding running (namely with our Compiler/CLI APIs)

src/compiler/config/test/validate-config.spec.ts Outdated Show resolved Hide resolved
src/compiler/config/outputs/index.ts Show resolved Hide resolved
@splitinfinities
Copy link
Contributor

splitinfinities commented Dec 3, 2021

Checked this branch out, linked it to a component library and both within npm run build and npm run start, the CLI produced a warning prior to the devServer process beginning as expected per AC. Passing the watch flag also only generates this warning once throughout the lifecycle of the --watch command, as anticipated.

Screen Shot 2021-12-03 at 3 49 44 PM

I have no AC for this warning to surface into a Browser's Console during the lifecycle of a watch command. I have no AC for this warning to fire off during an invoked command via Stencil's Core Compiler API.

So yeah, this looks great to me! :shipit:

Co-authored-by: Ryan Waskiewicz <ryanwaskiewicz@gmail.com>
Copy link
Contributor Author

@willmartian willmartian left a comment

Choose a reason for hiding this comment

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

Thanks for review; made appropriate changes.

src/compiler/config/outputs/index.ts Outdated Show resolved Hide resolved
src/compiler/config/test/validate-config.spec.ts Outdated Show resolved Hide resolved
@rwaskiewicz rwaskiewicz changed the title refactor(dist-custom-elements-bundle): add deprecation warning feat(dist-custom-elements-bundle): add deprecation warning Dec 6, 2021
@rwaskiewicz rwaskiewicz merged commit c7b07c6 into main Dec 6, 2021
@rwaskiewicz rwaskiewicz deleted the fix-stencil-259 branch December 6, 2021 15:02
@rwaskiewicz
Copy link
Member

This fix has been included in the v2.12.0 release.

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