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: support modern package exports syntax #5996

Conversation

louis-bompart
Copy link
Contributor

@louis-bompart louis-bompart commented Sep 24, 2024

What is the current behavior?

Currently, you cannot use packages that use modern module resolution based on subpath-exports in your Stencil Project1.
This is caused by the hard coding of Node10 for moduleResolution in Stencil.

GitHub Issue Number: #4229 (mitigation, not 💯 fix)

What is the new behavior?

Replace Node10 with Bundler. While it's not a "trivial" change, the impact for the end-user should be minimal (see Breaking change section)

Documentation

N.A.

Does this introduce a breaking change?

  • Yes
  • No

I ran some 'health-check' (running stencil compiler with & without my changes on packages that use various ways of defining sub-packages), and all projects that worked without my changes worked with it as well, without changes.

Moreover, from the TypeScript reference documentation, I reckon it is safe to say that Bundler is akin to a "superset" of Node10:
https://www.typescriptlang.org/docs/handbook/modules/reference.html#bundler
https://www.typescriptlang.org/docs/handbook/modules/reference.html#node10-formerly-known-as-node

Testing

In addition to the existing test (npm test), I ran some san-check. We have a package that we are migrating from 'hackish subpackages' (using subdirectory and package.json in them) to subpath exports in a major version, I used this package as an example and did the following tests:

To validate, check the linked repo (pay attention to the branch), npm i ; npm start

I'm not necessarily expecting this to be an 'LGTM & merge' but more a base to discuss how to improve Stencil so that it's not a blocker for good practices adoption (pure incompatibility with modern package practice is quite a sore point tbh), which is paramount IMHO for developer-oriented products.

Footnotes

  1. https://nodejs.org/api/packages.html#subpath-exports

@christian-bromann
Copy link
Member

@louis-bompart thanks for your contribution 🙏

Let us verify this with some of the upstream packages that rely on Stencil to check compatibility. From my point of view: if the breaking change is reasonable and doesn't impact the majority of users (as far as we can expect it) I would be in favor to move forward with this.

@christian-bromann christian-bromann added the Resolution: Needs Investigation This PR or Issue should be investigated from the Stencil team label Sep 24, 2024
@louis-bompart
Copy link
Contributor Author

Hey @christian-bromann 👋 Thanks for checking this out 🙏

I checked the ❌ in the CI and followed the trail.

I reckon this use case of rollup in the browser like in test/browser-compile/src/components/app-root/app-root.tsx is quite... unique 😄, and would not be something the "majority of users" would do. Moreover, users (well, outside Stencil itself) should be able to update.

I understand Stencil is in a tricky position regarding Rollup & the version they can use (#5274), so updating is not a solution in the present case.

From what I gather from v4.17.0 release notes, I reckon Stencil's goal is to replace Rollup by esbuild in the future.

With all that in mind, I'd propose a solution hack: using patch-package to tweak ever so slightly the package.json of rollup in your node_modules by applying the following commit rollup/rollup@2cca505.
This is the path of 'least resistance,' which I think could be acceptable, given where the project is headed (from what I understand).
This method's boon is that you can still use the same rollup version in production and in your tests.

If this last point is of no concern, you could leverage subpath imports to have 2 versions of rollup side-by-side in the project.
Just need to add something like that in the devDependencies:
`"rollup-test": "npm:rollup@^2.79.0"

And replace the import from "rollup" in the test with an import from "rollup-test."

What do you think?

@christian-bromann
Copy link
Member

christian-bromann commented Nov 20, 2024

I reckon Stencil's goal is to replace Rollup by esbuild in the future.

We do not plan to internally replace Rollup with Esbuild. We have replaced our internal bundler but right now Rollup is too integrated into Stencil that updating it is a better move forward than completely replacing it.

What do you think?

I think we should focus on the Rollup update to enable this feature. It is essential for a lot of other features in Stencil and we have put this on our roadmap to tackle it eventually. Any support and help would be much appreciated though.

Do you think we can close this or would you like to try a different approach?

@louis-bompart
Copy link
Contributor Author

I reckon Stencil's goal is to replace Rollup by esbuild in the future.

We do not plan to internally replace Rollup with Esbuild. We have replaced our internal bundler but right now Rollup is too integrated into Stencil that updating it is a better move forward than completely replacing it.

What do you think?

I think we should focus on the Rollup update to enable this feature. It is essential for a lot of other features in Stencil and we have put this on our roadmap to tackle it eventually. Any support and help would be much appreciated though.

Do you think we can close this or would you like to try a different approach?

That makes sense 👍 Let's close this for now, thanks for your answer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Needs Investigation This PR or Issue should be investigated from the Stencil team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants