-
Notifications
You must be signed in to change notification settings - Fork 171
Migrated vite to rollup #3060
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
Migrated vite to rollup #3060
Conversation
|
|
❌ PR title doesn't follow Conventional Commits specification. Details: |
| @@ -0,0 +1,27 @@ | |||
| // Re-export Base components with their original names to ensure they're available | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One drawback of this approach is for blade and consumer to be on same svelte version as far as I have observed.
Another approach could be to let consumer compile the svelte
module.exports = { module: { rules: [ { test: /\.svelte$/, // ⚠️ Must include node_modules/@razorpay include: [ path.resolve(__dirname, 'src'), /node_modules\/@razorpay/ // Add this! ], use: { loader: 'svelte-loader', options: { preprocess: sveltePreprocess() } } } ] } };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is re-exports related to binding consumers with same svelte version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad not a svelte version issue. The re-export was needed so that when svelte 5 compiles for eg Button component which needs basebutton it is able to find both. Without re-exporting it was compoiling button but only loading the import statement of Basebutton and not the actual component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh. Is it common known issue? Like why does svelte not follow the import statements while compiling 🤔 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I checked this started happening in svelte 5 where there are some issues in treeshaking. The above error was due to the version on which blade i running svelte. As soon as I upgraded it to match consumer it got fixed.
But yeah, the re-exports was still an issue so had to use re-exports to fix the reference errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes yes I meant is there any open issue in svelte repo for this usecase? like its common usecase for all consumers right?
| // Workaround for Svelte tree-shaking: reference imported classes to prevent removal | ||
| const buttonContentClass = _buttonContentClass; | ||
| const buttonIconClass = _buttonIconClass; | ||
| const loadingSpinnerClass = _loadingSpinnerClass; | ||
| const loadingClass = _loadingClass; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we not want it to treeshake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basicall what is happening is while compiling i
import {
ButtonContentClass
} from '@rzorpay/blade-core.styles'
Svelte 5 compiler treat this as unused import since it;s reference is only made in the html template and not JS execution script.
import * as styles from '@razorpay/blade-core/styles'; we can do this as well but this would mean there is no treeshaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh interesting. This seems like a common usecase though no? is there some open issue on svelte side to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could nto find any open issue on svelte side but solved it with this approach #3062
| @@ -0,0 +1,27 @@ | |||
| // Re-export Base components with their original names to ensure they're available | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is re-exports related to binding consumers with same svelte version?
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7c1d2c6:
|
c6d6ecb to
322ee50
Compare
57f80ae to
da2dcdd
Compare
saurabhdaware
left a comment
There was a problem hiding this 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. Added one followup comment.
Can you somewhere maybe add a README on how to use this library? you can add a Installation section in Svelte storybook I guess the way we have in react storybook?
You can do it in separate PR if you want
| @@ -0,0 +1,27 @@ | |||
| // Re-export Base components with their original names to ensure they're available | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes yes I meant is there any open issue in svelte repo for this usecase? like its common usecase for all consumers right?
* Resolved all design review comments * Resolved all design review comments * Added Readme and installation docs * Updated docs
* Reverting blade-core from blade-react * Reverting blade-core from blade-react * Upgraded to node 20 * Upgraded to node 20 * Migrated vite to rollup (#3060) * Migrated vite to rollup * Updated publish workflow * PR comments resolved * PR comments resolved * Resolved all design review comments (#3062) * Resolved all design review comments * Resolved all design review comments * Added Readme and installation docs * Updated docs
* Added Link Component to Svelte | Cursor Rules for Effecient React to Svelte Migration * Added Link Component to Svelte | Cursor Rules for Effecient React to Svelte Migration * PR comments resolved * Added Base Text Component | Migrated styles to use css classes | updated link to use BaseText * Added Heading | Code | Text component | Refactored to use default css styling and create classes * CSS STyling migration | Migrated all css logic to blade-core * PR comments resolved * Added Button.Svelte * Added PostCSS support | Added Loader component for Button * Added amount component * Added storybook implementation * Added storybook implementation * Added storybook implementation * Added storybook implementation * Added storybook implementation * Added storybook implementation * Fixed build issues * Fixed build issues * Fixed build issues * Fixed build issues * Fixed storybook workflow * Storybook playground fixes * Storybook playground fixes * Fixed fonts * Pr comments resolved * Reverting blade-core from blade-react (#3059) * Reverting blade-core from blade-react * Reverting blade-core from blade-react * Upgraded to node 20 * Upgraded to node 20 * Migrated vite to rollup (#3060) * Migrated vite to rollup * Updated publish workflow * PR comments resolved * PR comments resolved * Resolved all design review comments (#3062) * Resolved all design review comments * Resolved all design review comments * Added Readme and installation docs * Updated docs

Description
Changes
Additional Information
Component Checklist