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: removed lucide aliases #53789

Closed
wants to merge 6 commits into from
Closed

Conversation

jguddas
Copy link

@jguddas jguddas commented Aug 9, 2023

#53483 introduced support for lucide aliases in next.js, to not have to maintain this mapping in two places lucide will have a file for every alias.

lucide-icons/lucide#1486

lucide-icons/lucide#1486

Lucide is now having a file for every alias that just contains `export {default} from "./icon-name"`.
@ijjk ijjk added the type: next label Aug 9, 2023
'(Stars|LucideStars|StarsIcon)':
'modularize-import-loader?name={{ member }}&from=default&as=default&join=../esm/icons/sparkles!lucide-react',
Copy link
Author

@jguddas jguddas Aug 13, 2023

Choose a reason for hiding this comment

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

Now there is a /esm/icons/stars.js file containining a simple reexport from ./sparkles.js.

/**
 * lucide-react v0.267.0 - ISC
 */

export { default } from './sparkles.js';
//# sourceMappingURL=stars.js.map

https://www.npmjs.com/package/lucide-react?activeTab=code

@jguddas jguddas marked this pull request as ready for review August 13, 2023 20:18
@vercel-spaces-staging
Copy link

Notifying the following users due to files changed in this PR based on this repo's notify modifiers:

@timneutkens, @ijjk, @shuding, @styfle, @huozhi:

packages/next/src/server/config.ts

1 similar comment
@vercel-spaces
Copy link

vercel-spaces bot commented Aug 13, 2023

Notifying the following users due to files changed in this PR based on this repo's notify modifiers:

@timneutkens, @ijjk, @shuding, @styfle, @huozhi:

packages/next/src/server/config.ts

@jd-carroll
Copy link

@ztanner we are really excited to use #54016 but will not be able to use the latest due to this issue. Any chance we can get this PR merged soon?

@itsjavi
Copy link

itsjavi commented Aug 20, 2023

lucide-react imports seem broken until this is merged, or that's what I understood from Lucide's discord

Copy link
Member

@shuding shuding left a comment

Choose a reason for hiding this comment

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

Can we fix the wrong transform rule instead of removing all of them? This breaks the optimization for older versions of Lucide React.

@itsjavi
Copy link

itsjavi commented Aug 22, 2023

Can we fix the wrong transform rule instead of removing all of them? This breaks the optimization for older versions of Lucide React.

The problem is that every time something changes in lucide-react, the library maintainers will have to come here and fix the rules again if they break.

I think it's ok for older versions to not be optimized, as long as they are not broken.

@jguddas
Copy link
Author

jguddas commented Aug 22, 2023

This breaks the optimization for older versions of Lucide React.

@shuding old versions of lucide are already broken, since they use ./icons rather than ../esm/icons.

There are only very view versions that have both ../esm/icons but don't have the aliases i.e. ../esm/icons/stars.js that @ericfennis and I added so we don't have to maintain them in both places.

Keeping those aliases here will break as soon as we add a starts icon to lucide for example.

If there are 250 or 253 prerelease versions of lucide that don't work is IMO okay considering this problem.

shuding added a commit that referenced this pull request Aug 25, 2023
…ig (#54572)

## Implementation

Base on #54530, we're implementing a `optimize_barrel` transform to
optimize barrel files to only export the names we need. If the
transformed file isn't a "barrel file", we just re-export the names from
it without any transformation.

Take `lucide-react` as an example, with #54530 we are able to transform

```js
import { IceCream } from 'lucide-react'
```

to 

```js
import { IceCream } from '__barrel_optimize__?names=IceCream!=!lucide-react?__barrel_optimize_noop__=IceCream'
```

And then, we apply that new request with a new Webpack module rule to
use the SWC loader with option `optimizeBarrelExports: ['IceCream']`,
which eventually got passed to this new `optimize_barrel` transform and
does the optimization.

## Notes

We'll have to add a new `getModularizeImportAliases` alias list to map
`lucide-react` to the ESM version, as we have the `['main', 'module']`
resolve order for the server compiler. Otherwise this optimization
doesn't work in that compiler.

There's no e2e test added because it's already covered by the
`modularize-imports` test as we removed the default `lucide-react`
transform rules and it still works.

We'll need to test other libs before migrating them to the new
`optimizePackageImports` option.

---

Closes #54571, closes #53605, closes #53789, closes #53894, closes
#54063.
@github-actions github-actions bot added the locked label Sep 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants