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.

Sign up for GitHub

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

Jump to bottom

fix: fix imports for AMD/CommonJs #11618

Merged
merged 7 commits into from
Jan 10, 2019
Merged

fix: fix imports for AMD/CommonJs #11618

merged 7 commits into from
Jan 10, 2019

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Nov 29, 2018 •
edited
Loading

Description

Fix imports of the UMD files for AMD and CommonJS environments.

Changes:

  • Make the Foundation plugins looking for ./plugin instead of plugin for AMD/CommonJs formats.
  • Add documentation about the UMD configuration.
  • Refactor and add documentation to the utilities used for the UMD build.

Todo:

  • Add tests

Types of changes

  • Documentation
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (anything that would change an existing functionality)
  • Maintenance (refactor, code cleaning, development tools...)

Checklist

  • I have read and follow the CONTRIBUTING.md document.
  • The pull request title and template are correctly filled.
  • The pull request targets the right branch (develop or develop-v...).
  • My commits are correctly titled and contain all relevant information.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).

@ncoden
chore: fix plugins UMD external names
4d9270f 
Make the plugins looking for "./plugin" instead of "plugin" for AMD/CommonJs formats
@ncoden
docs: add documentation for the webpack UMD configuration
7f5aa78
@ncoden
chore: fix plugins' UMD root variables names
4124057 
Use names without "./" when searching for modules as root variables (in module-less environments) to correctly match the generated root variables, which are the name of the file (without any prefix).
@ncoden ncoden added this to the 6.5.2 milestone Nov 29, 2018
@ncoden ncoden mentioned this pull request Nov 29, 2018
Closed
@ncoden ncoden added the WIP label Nov 29, 2018
@ncoden
Copy link
Contributor Author

ncoden commented Dec 23, 2018

I added tests for the AMD import only because Foundation is not compatible for the native Node environment (without browser features) and there is no implementation for CommonJs in the browser.

@ncoden ncoden removed the WIP label Dec 23, 2018
@ncoden ncoden requested a review from DanielRuf December 23, 2018 21:42
DanielRuf
DanielRuf reviewed Dec 24, 2018
View reviewed changes
DanielRuf
DanielRuf reviewed Dec 24, 2018
View reviewed changes
DanielRuf
DanielRuf reviewed Dec 24, 2018
View reviewed changes
DanielRuf
DanielRuf reviewed Dec 24, 2018
View reviewed changes
DanielRuf
DanielRuf reviewed Dec 24, 2018
View reviewed changes
DanielRuf
DanielRuf reviewed Dec 24, 2018
View reviewed changes
DanielRuf
DanielRuf reviewed Dec 24, 2018
View reviewed changes
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

See my comments.
In general we should try to use rollup as best as possible as this provides the UMD and AMD wrappers.

https://rollupjs.org/guide/en

@Lemmork
Copy link

Lemmork commented Jan 8, 2019

I checked out this pull request locally and the js files have version 6.4.3.
Shouldn't this pull request be merged to branch develop-v6.5 ?

@ncoden
Copy link
Contributor Author

ncoden commented Jan 9, 2019 •
edited
Loading

@Lemmork develop is ahead of develop-v6.5 but has not been released, this is why the version has not been bumped. It is used to prepare v6.6. See https://github.com/zurb/foundation-sites/blob/develop/CONTRIBUTING.md#git-workflow.

@ncoden ncoden force-pushed the chore/fix-externals-config branch from ec34586 to a69a577 Compare January 9, 2019 21:02
@ncoden
Copy link
Contributor Author

ncoden commented Jan 9, 2019 •
edited
Loading

@DanielRuf It would be great to fully migrate from Webpack to Rollup for the javascript, but this adds a lot of work. If you think it worth it, go ahead and submit a PR. I'm also curious to see how you will test the AMD bundle import.

I'm merging this for now to release a patch quickly, but some refactor to have a lighter build process in future versions would be welcome!

@DanielRuf
Copy link
Contributor

Right. Currently I'm mainly working on ecommerce projects at work but there should be a new Foundation based project in the next weeks where I can test the bundles with the colleagues in the project initialization and setup phase.

@ncoden ncoden merged commit 1bf51b2 into foundation:develop Jan 10, 2019
ncoden added a commit that referenced this pull request Jan 11, 2019
@ncoden
Use pull request #11618 from ncoden/chore/fix-externals-config for v6…
bef88c4 
….5.0

4d9270f chore: fix plugins UMD external names
7f5aa78 docs: add documentation for the webpack UMD configuration
4124057 chore: fix plugins' UMD root variables names
3fa6741 test: add test for imports via AMD
91353db docs: improve description for the webpack externals config in build script
a69a577 docs: improve docs for the root namespace in build script
5e42209 docs: improve docs of the "getUmdEntry" build utility

Signed-off-by: Nicolas Coden <[email protected]>
@ncoden ncoden mentioned this pull request Jan 12, 2019
Merged
11 tasks
@joeworkman
Copy link
Member

This pull request has been mentioned on Foundation Open Source Community. There might be relevant details there:

https://foundation.discourse.group/t/foundation-for-sites-v6-6-0-is-here-farout/30/1

This was referenced Mar 10, 2020
Closed
Closed
This was referenced Apr 22, 2020
Open
Open
Merged
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRuf DanielRuf DanielRuf left review comments

@Lemmork Lemmork Lemmork approved these changes

Assignees
No one assigned
Labels
bug fix 🚚 build
Projects
None yet
Milestone
6.5.2
Development

Successfully merging this pull request may close these issues.

missing files foundation.core.utils.js and foundation.core.plugin.js in dist/js/plugins
4 participants
@ncoden @Lemmork @DanielRuf @joeworkman