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

Production build includes two copies of preact? #126

Closed
ethanroday opened this issue Jun 20, 2017 · 11 comments
Closed

Production build includes two copies of preact? #126

ethanroday opened this issue Jun 20, 2017 · 11 comments

Comments

@ethanroday
Copy link
Contributor

Steps to reproduce:

  1. npm install -g preact-cli
  2. preact create <app>
  3. cd <app>
  4. preact build --json
  5. Inspect stats.json using a Webpack stats analyzer (this one is from webpack-bundle-analyzer):

image

Expected behavior:

The preact library should only be included once in bundle.js.

Actual behavior:

It is included twice.

@developit
Copy link
Member

Ah I think this is caused by preact-cli including preact as a dependency now. I think we can fix this by using an absolute path to Preact in the aliases setup.

Curious if @lukeed or @thangngoc89 see any issues in doing that.

@lukeed
Copy link
Member

lukeed commented Jun 21, 2017

@developit Should be no issue since webpack handles everything (internally) with absolute paths anyway.

@developit
Copy link
Member

Would love a PR for this btw if anyone has a few minutes. Otherwise I'll try to get around to it whenever I can.

@reznord
Copy link
Member

reznord commented Jun 23, 2017

I have given this a shot and it worked for me:
preact$: resolve(__dirname, '../../node_modules/preact/dist/preact.min.js'),

Is there a better way to do this? I am attaching the JSON file which I got while testing.

Please find the screenshot of the webpack bundle analyzer,

screen shot 2017-06-23 at 10 36 30 am

stats.json file: stats.json.zip

@developit @lukeed any suggestions? If this is fine, I would be happy to submit a PR.

@developit
Copy link
Member

We should have the alias attempt to use preact from $CWD/node_modules/preact and then maybe fall back to preact-cli's version. Otherwise that looks good, would love a PR.

@reznord
Copy link
Member

reznord commented Jun 23, 2017 via email

@reznord
Copy link
Member

reznord commented Jun 23, 2017

We should have the alias attempt to use preact from $CWD/node_modules/preact and then maybe fall back to preact-cli's version

Is this fine?

preact$: isProd ? resolve(__dirname, '../../node_modules/preact/dist/preact.min.js') : 'preact',

@lukeed
Copy link
Member

lukeed commented Jun 23, 2017

How about:

preact$: isProd ? resolve(env.cwd, 'node_modules/preact/dist/preact.min.js') : 'preact'

@reznord
Copy link
Member

reznord commented Jun 23, 2017

Cool, I did the same thing after I commented 😅

@reznord
Copy link
Member

reznord commented Jun 23, 2017

@developit @lukeed please have a look at it.

Yaaay, bundle size reduced from 26.8kb to 18.9kb.

@reznord
Copy link
Member

reznord commented Jun 23, 2017

@lukeed it failed 8 tests !! not sure why? Can you please have a look at the log?

https://paste.fedoraproject.org/paste/~djRyBcRUltRzRRaON3~qg

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

No branches or pull requests

4 participants