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

[REGRESSION] Calling $(document).foundation() fails in production builds #11586

Closed
3 tasks done
josephcsible opened this issue Nov 13, 2018 · 8 comments · Fixed by #11607
Closed
3 tasks done

[REGRESSION] Calling $(document).foundation() fails in production builds #11586

josephcsible opened this issue Nov 13, 2018 · 8 comments · Fixed by #11607
Assignees

Comments

@josephcsible
Copy link

What should happen?

Calling $(document).foundation() in production React app builds should work, as it did in Foundation 6.5.0-rc.2.

What happens instead?

Calling $(document).foundation() in production React app builds results in an error like TypeError: w(...)(...).foundation is not a function.

Test Case and/or Steps to Reproduce (for bugs)

How to reproduce:

  1. Run npx create-react-app my-app
  2. Run cd my-app
  3. Run yarn add jquery foundation-sites@6.5.0
  4. Add import 'foundation-sites';, import $ from 'jquery';, and $(document).foundation(); to src/index.js
  5. Run yarn build
  6. Run yarn global add serve
  7. Run serve -s build
  8. Visit http://localhost:5000
  9. Look in the browser console

Context

I’d like to be able to use the latest version of Foundation in a production React app. This is a regression, as my reproduction steps don't cause the error with Foundation 6.5.0-rc.2. Also, the error only occurs in production builds, not in development (which you can see by doing yarn start in place of steps 5, 6, and 7, and then going to http://localhost:3000).

Your Environment

  • Foundation version(s) used: 6.5.0
  • Browser(s) name and version(s): Firefox 60.2.0esr, Chrome 70.0.3538.102
  • Device, Operating System and version: Tested in Docker container node:10-alpine
  • Link to your project: N/A

Checklist

  • I have read and follow the CONTRIBUTING.md document.
  • There are no other issues similar to this one.
  • The issue title and template are correctly filled.
@ncoden ncoden self-assigned this Nov 13, 2018
@ncoden
Copy link
Contributor

ncoden commented Nov 13, 2018

Hi @josephcsible 👋,

Could you tell me:

  • Does it work with v6.5.0-rc3 ?
  • Does it work with v6.5.0-rc4 ?

@josephcsible
Copy link
Author

It doesn't work with either of those versions.

@ncoden
Copy link
Contributor

ncoden commented Nov 13, 2018

Ok, thank you. I think it comes from #11445 and the fact that Foundation is not imported because it is not used directly, but I'll do some checks tonight to verify that.

@ncoden
Copy link
Contributor

ncoden commented Nov 13, 2018

@josephcsible

For now you can try the following:

import $ from 'jquery';

(...)

// Foundation JS relies on a global varaible.
// In ES6, all imports are hoisted to the top of the file
// so if we used `import` to import Foundation, it would
// execute earlier than we have assigned the global
// variable. This is why we have to use CommonJS require()
// here since it doesn't have the hoisting behavior.
window.jQuery = $;
require('foundation-sites');

This will also force your build to import Foundation, because there is no tree shaking with CommonJS.

See https://github.com/ncoden/react-foundation-sites-app

@ncoden
Copy link
Contributor

ncoden commented Nov 13, 2018

Note to myself: A lot of people are using Foundation via its jQuery integration even in a ESM environment, thus rely on Foundation side-effects (on jQuery). This could be a reason not to consider Foundation as without side-effects and to remove sideEffects: false from package.json.

Unless we find a better way to implement that, a true tree shaking support will require some changes in the way Foundation can be imported, initialized and used.

@josephcsible
Copy link
Author

A lot of people are using Foundation via its jQuery integration

Is there an alternative to that? It's the only way I've really seen documented.

@ncoden
Copy link
Contributor

ncoden commented Nov 13, 2018

Is there an alternative to that? It's the only way I've really seen documented.

Well it is possible to manually initialize the DOM:

Foundation.reflow($(document));

Or to manually instantiate plugins:

var accordion = new Foundation.Accordion($('#some-accordion'), options);

https://foundation.zurb.com/sites/docs/javascript.html

But I guess this is not documented enough.

@ncoden
Copy link
Contributor

ncoden commented Nov 16, 2018

I keep this issue open because there is at least the documentation to update.

ncoden added a commit to ncoden/foundation-sites that referenced this issue Nov 22, 2018
Despite the previous attempts to make Foundation compatible with tree shaking (foundation#11445, foundation#11497 and foundation#11508), Foundation is used with side-effects (on jQuery) and cannot be considered as "without side-effects" for tree-shaking purposes.

This commit remove the "sideEffects: false" label and thus disable tree-shaking support, until the Foundation API and usage is changed to not implicitely rely on the global jQuery object.

Closes foundation#11586

Related to:
- foundation#11508
- foundation#11497
- foundation#11445
ncoden added a commit to ncoden/foundation-sites that referenced this issue Nov 22, 2018
Despite the previous attempts to make Foundation compatible with tree shaking (foundation#11445, foundation#11497 and foundation#11508), Foundation is used with side-effects (on jQuery) and cannot be considered as "without side-effects" for tree-shaking purposes.

This commit remove the "sideEffects: false" label and thus disable tree-shaking support, until the Foundation API and usage is changed to not implicitely rely on the global jQuery object.

Closes foundation#11586

Related to:
- foundation#11508
- foundation#11497
- foundation#11445
ncoden added a commit that referenced this issue Nov 23, 2018
ncoden added a commit that referenced this issue Jan 11, 2019
….5.0

d8bcc3a chore: temporary disable tree-shaking #11586

Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
@ncoden ncoden mentioned this issue Jan 12, 2019
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants