Closed zoeyfrisart closed 4 months ago
@CodiumAI-Agent /review
β±οΈ Estimated effort to review [1-5] | 4, due to the extensive changes across multiple frameworks and the introduction of new functionality that requires thorough testing across different environments. |
π§ͺ Relevant tests | No |
π Possible issues | Performance Concern: The implementation iterates through multiple arrays using nested loops which could lead to performance issues on pages with a large number of flags. |
Code Duplication: Similar flag rendering logic is repeated across different files and frameworks (React, Next.js, Remix, Gatsby), which could be centralized to improve maintainability. | |
π Security concerns | No |
relevant file | test-applications/create-react-app/src/App.js |
suggestion | Consider using React's useMemo hook to memoize the flags array to avoid recalculating it on every render, enhancing performance. [important] |
relevant line | const flags = [ |
relevant file | test-applications/remix/app/routes/_index.tsx |
suggestion | Extract the flag rendering logic into a separate component or custom hook to reduce code duplication and improve readability. [important] |
relevant line | const flags: Flags[] = [ |
relevant file | test-applications/next-app-router/src/app/page.tsx |
suggestion | Use a configuration file for flags data to simplify updates and maintenance. Import this configuration instead of hardcoding the flags array in multiple files. [medium] |
relevant line | const flags: Flags[] = [ |
Is this going to be merged at some point in the near future?π€ @zoeyfrisart
@tnmdynamiq I certainly hope so, but it needs a review from the rest of our dev team. I'll see if I can push for some time in the upcoming week(s) to review this pull request.
@tnmdynamiq I certainly hope so, but it needs a review from the rest of our dev team. I'll see if I can push for some time in the upcoming week(s) to review this pull request.
Thank you very much for clarifying! Much appreciated, I will keep my eye out for it.
would love for this to be merged soon
@zoeyfrisart Would it be possible to release this as a @next version on npm or something like that?
This would allow the community to use & test it so you could get more feedback while we wait for the rest of your dev team to review this.
/** @type {import('next').NextConfig} */
const withTM = require('next-transpile-modules')(['@stampmyvisa/react-flagpack']);
module.exports = withTM({
trailingSlash: false,
output: "export",
images: { unoptimized: true },
webpack: (config, { isServer }) => {
config.module.rules.push({
test: /\.svg$/,
include: /node_modules\/@stampmyvisa\/react-flagpack\/dist\/flags/,
use: [
{
loader: 'file-loader',
options: {
name: '[path][name].[hash].[ext]', // Update this line
outputPath: 'static/flags/',
publicPath: '/_next/static/flags/',
emitFile: !isServer,
},
},
],
});
return config;
},
})
This is how I'm fixing it with the existing package
This is how I'm fixing it with the existing package
I'm using Vite, but I'll try to see if I can make it work in a similar way. Thanks for your solution!
Hi all, We'll be releasing this later today on NPM
Hi all, this is now published on NPM, you can install it by bumping your version to ^2.0.2
, be sure to follow the migration guide https://github.com/Yummygum/react-flagpack/tree/v2.0.2?tab=readme-ov-file#migrating-to-200
Apologies for the delay in this release becoming available to the general public!
This version implements a CLI that will auto-inject the flags into the static folder of a given project. (Could be improved in the future to auto-detect the framework/static folder of a project)
This rework works by removing the dynamic required implementation that was not only causing issues but was also not tree-shaking correctly when looking at the resulting bundle.
This new implementation should work properly in:
Other frameworks have yet to be tested.
Also, I took the time to refactor the build process to vote instead of the roll-up. Also exports the props interface to make the component easier to use with TypeScript
CLOSES: #69 #70 #58 #55 #47 #40 Possibly fixes: #45 & #46 but that needs more testing
This change is a breaking change that would also require changes to the flatpack website documentation (and requires changes on the user's part). I have marked this as a breaking change, which means the version will increase to 2.0.0