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

Bundle for CDN #5796

Merged
merged 4 commits into from
Dec 18, 2016
Merged

Bundle for CDN #5796

merged 4 commits into from
Dec 18, 2016

Conversation

nealeu
Copy link
Contributor

@nealeu nealeu commented Dec 15, 2016 •
edited by oliviertassinari
Loading

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

This is a re-work of ideas from #4342, but based on 'next' branch so that things can work without the react-tap-event-plugin.

I'll attempt to use this for our development build, but confine this PR to what's needed for the CDN bundle

@muibot muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Dec 15, 2016
@nealeu nealeu force-pushed the bundle-for-cdn branch 2 times, most recently from b4b4a6b to 400d1a2 Compare December 15, 2016 22:23
oliviertassinari
oliviertassinari reviewed Dec 15, 2016
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Sounds like a good idea to build a UMD package.
Using Rollup as pouchdb would most likely provide a more performance bundle, but let's keep thing simple with webpack for now 👍 .

README.md
@@ -1,3 +1,18 @@
#### UMD Packaging
Copy link
Member

Choose a reason for hiding this comment

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

I would move that section lower in the README.

webpack.config.js
},
],

// plugins: [
Copy link
Member

Choose a reason for hiding this comment

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

Why keeping that commented block?

webpack.config.js
{
test: /(\.jsx|\.js)$/,
loader: 'babel',
exclude: /(node_modules|bower_components)/,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need to exclude the bower_components. We don't use bower.

@nealeu
Copy link
Contributor Author

nealeu commented Dec 16, 2016

@oliviertassinari Thanks for the feedback. I'll update with fixes for those items.

@muibot muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 16, 2016
@nealeu
Copy link
Contributor Author

nealeu commented Dec 16, 2016

Hold on... the rebase shouldn't have done that. Will try again

@muibot muibot added PR: Needs Review and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Dec 16, 2016
@nealeu
Copy link
Contributor Author

nealeu commented Dec 16, 2016

@oliviertassinari I think that's now ready for you.

oliviertassinari
oliviertassinari requested changes Dec 17, 2016
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Thanks for continuing the old PR!
Any idea what's breaking the build? I haven't figured out.

.gitignore
@@ -23,3 +24,4 @@ jsconfig.json

# tmp
tmp*
*~
Copy link
Member

Choose a reason for hiding this comment

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

What is it for? Shouldn't it be in your user gitignore?

README.md
For using with React and React DOM from a CDN or as separate minified scripts, you can build
files with UMD module support as follows:

npm install
Copy link
Member

Choose a reason for hiding this comment

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

I would rather be more explicit with:

```sh
npm install
```

README.md

To see what's going on under the hood, you can use webpack directly:

webpack --display-reasons --display-modules --progress --optimize-minimize --colors --entry ./src/index.js
Copy link
Member

Choose a reason for hiding this comment

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

Same as before for explicitness

webpack.config.js
},
},
],

Copy link
Member

Choose a reason for hiding this comment

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

Could you remove those blank lines?

webpack.config.js
module: {
loaders: [
{
test: /(\.jsx|\.js)$/,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need jsx: /\.js$/,

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 17, 2016 •
edited
Loading

Actually, that was an npm cache issue, the build is green now :).

@oliviertassinari oliviertassinari merged commit d55b144 into mui:next Dec 18, 2016
@oliviertassinari
Copy link
Member

I'm merging it, I'm gonna address the comment. Thanks!

@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Dec 18, 2016
@oliviertassinari
Copy link
Member

I have made two more commits:

  • 0db2aca: takes my feedback into action
  • 8b80668: realized that having a single script would be better to reduce confusion

@oliviertassinari oliviertassinari mentioned this pull request Dec 18, 2016
Closed
@ricardopolo
Copy link

Nice, now let's put ther latest version In CDNJs with auto update for each release 😀

@ravilution
Copy link

Hi all, sounds like you guys have successfully created the CDN UMD version of the files. Can you please share a URL? Thanks in advance.

@oliviertassinari
Copy link
Member

@ravilution It's on the homepage:
capture d ecran 2018-07-15 a 01 19 57

@ravilution
Copy link

@oliviertassinari thanks man. I missed that place. I was looking here. My bad.

If I have to use material icons from CDN, there is no UMD file for that, right? I will have to use below font based implementation. Correct me if am wrong.

<link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons">

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 15, 2018 •
edited
Loading

@ravilution font icon is the way to go with CDN 👍 .

@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari requested changes

Assignees
No one assigned
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nealeu @oliviertassinari @ricardopolo @ravilution @muibot @zannager