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

fix(core): add an export map to import individual components #29717

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

christian-bromann
Copy link
Member

@christian-bromann christian-bromann commented Jul 16, 2024

What is the current behavior?

The core package doesn't provide an export map, which prevents modern code bases to import specific components.

fixes #29716

What is the new behavior?

This patch contains:

  • a switch from node to bundler in the `moduleResolution field, both resolutions are very similar - changing this has no impact on the user as it is only for internal development
  • adding export entries for individual components to allow packages using modern module resolutions to import individual components

Does this introduce a breaking change?

  • Yes
  • No

This should not impact users at all but enable them to use a different module resolution. However, I may not be too familiar how Ionic components are imported, therefor it would be good to do some thorough testing.

Other information

n/a

@christian-bromann christian-bromann requested a review from a team as a code owner July 16, 2024 18:07
Copy link

vercel bot commented Jul 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 17, 2024 6:16pm

core/package.json Show resolved Hide resolved
core/package.json Show resolved Hide resolved
@github-actions github-actions bot added the package: vue @ionic/vue package label Jul 16, 2024
@christian-bromann
Copy link
Member Author

Thanks @sean-perkins for the feedback, I applied suggested improvements.

@sean-perkins
Copy link
Contributor

@christian-bromann it looks like the docs.json still needs to be added to the package.json exports to resolve the Vue related build issues.

For the angular build issues/test issues, you should see this error:

Error: Module not found: Error: Package path ./loader

we should export the loader path as well for the lazy loaded distribution.

Copy link
Contributor

@sean-perkins sean-perkins 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 to me 👍

We should definitely get someone from the Ionic Framework team to look at this for visibility to the change/impacts.

There is some risk that we are missing exporting an obscure build file that is not leveraged in Ionic Framework's packages/test apps, but a developer is making use of. I still think we could publish this as a patch or minor and if those cases are reported, we add the missing exports paths/evaluate what files are being consumed.

@sean-perkins sean-perkins requested review from brandyscarney and thetaPC and removed request for joselrio July 19, 2024 13:58
"import": "./components/ion-accordion.js",
"types": "./components/ion-accordion.d.ts"
},
"./ion-avatar": {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for not including ion-action-sheet, ion-alert, and others that are missing?

@muuvmuuv
Copy link

muuvmuuv commented Sep 26, 2024

Will this also fix the described code-splitting issue here #28574 ? Sounds like with that change I am able to just use a component from one exported path instead of just @ionic/angular/standalone, right?
ok nvm I just tested removing fesm and export datetime explictly it does not change the build outcome or put datetime in a separate chunk file, guess its up to the bundlers... looking at you rspack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: update moduleResolution from node to node16 or bundler
4 participants