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

Reconfigure webpack for pollyfilling and tree shaking #37

Merged
merged 5 commits into from Oct 28, 2020

Conversation

skarahoda
Copy link
Contributor

This PR applies guidelines for tree shaking

@skarahoda skarahoda requested a review from a team October 20, 2020 13:51
@skarahoda skarahoda self-assigned this Oct 20, 2020
Copy link
Member

@dirtybit dirtybit left a comment

Choose a reason for hiding this comment

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

LGTM with comments addressed.

src/config/webpack.common.js Outdated Show resolved Hide resolved
@@ -17,16 +17,22 @@ const copies = {
const { eslintConfig, main: entryPoint } = require(resolveExtensionPath("package.json"));
const eslintEnabled = eslintConfig || fs.readdirSync(extensionPath).find(f => f.startsWith(".eslintrc"));
const jsLoaders = [{
loader: require.resolve("babel-loader"),
Copy link
Member

Choose a reason for hiding this comment

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

If I remember it right, this helps when you run zem locally within its own directory.

How is your local development workflow? If we have a better way to run/test zem locally we can get rid of this.

☝️ We have another require.resolve usage in this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found several places that we use require.resolve and fix it. As far as I understand require.resolve import the module and return the file path instead of module itself. In our case this is redundant, since webpack can resolve the module from file name.

We also use it at manifest-builder: delete require.cache[require.resolve(pkgInfoPath)];. I'm not sure that we should remove from here also. That's why I didn't remove it.

"@babel/preset-env",
{
useBuiltIns: "usage",
corejs: 3,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add core-js@3 to the list of "dependencies" of zem since this is going to be needed in case a feature that requires polyfill is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already added. "core-js": "^3.6.4" 🤔

skarahoda and others added 2 commits October 28, 2020 09:20
Co-authored-by: Sertac Olgunsoylu <dirtybit@users.noreply.github.com>
@dirtybit dirtybit merged commit bddb3c1 into master Oct 28, 2020
@dirtybit dirtybit deleted the feature/polyfill-and-tree-shake branch October 28, 2020 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants