-
Notifications
You must be signed in to change notification settings - Fork 0
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
Reduce bundle size fortawesome #1289
Changes from all commits
8e7f484
d045813
c3e66d2
53151ec
c5dd270
9e6fa69
6cc8d5c
bbf5d99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import { | |
type ColProps as ReactBootstrapColProps, | ||
} from 'react-bootstrap'; | ||
|
||
import { useDeprecationWarning } from 'src/utils'; | ||
import { useDeprecationWarning } from '../utils'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This actually failed for me as well locally so using relative (looks like a new addition that wasn't released yet to rs repo)
When building rs shakapacker There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @jeffbax (and this is example from a different PR from a couple of days ago that would make a rs build fail if we tried to merge newest DS to rs) with relative imports to utils / icons in DS, RS build works. The problem here is defining aliases for internal of DS package that will also work with RS build. That could be looked at when we incorporate rollup as a follow on, but the PR in current states works with rs build. |
||
|
||
export type ColProps = { | ||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Keep this eslint-disable here. FontAwesome imports should only be happening | ||
// in this file and nowhere else in the codebase. | ||
// | ||
// Any direct import from '@fortawesome/free-brands-svg-icons' will create bloat in | ||
// our overall bundle size. | ||
|
||
/* eslint-disable no-restricted-imports */ | ||
|
||
import { faGoogle } from '@fortawesome/free-brands-svg-icons/faGoogle'; | ||
import { faFacebook } from '@fortawesome/free-brands-svg-icons/faFacebook'; | ||
import { faLinkedin } from '@fortawesome/free-brands-svg-icons/faLinkedin'; | ||
import { faTwitter } from '@fortawesome/free-brands-svg-icons/faTwitter'; | ||
|
||
export { | ||
faGoogle, | ||
faFacebook, | ||
faLinkedin, | ||
faTwitter, | ||
}; |
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.
Using relative imports; Locally I kept getting
When building rs shakapacker
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.
We can look at absolute imports without reexporting this as a nice to have in the follow up
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.
I think we can address the best course of actions for relative imports in a follow up PR; I was reading up about it and this may be tricky to get done. Adding alias in RS shakapacker config doesn't seem like a good solution
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.
is DS wired up to respect
ts
files? I'm assuming it is... but if it only had TSX until now then that could be why its not resolvingThere 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.
I think so; we've had
.ts
files here for a while. Based on what I was reading, it might seem like webpack/shaka might also have be aware of the correct alias configuration for bundling/resolving modules (which that seems weird to add an alias in shaka config in rs repo). So it may not be enough to specify an alias in.babelrc
file in DS repo. Happy to look more into this in a separate PRThere 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.
is that an error with rails server, or building DS?
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.
rails server; there's no error building DS