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

[Core] Improve our import path #2679

Closed
oliviertassinari opened this issue Dec 28, 2015 · 20 comments
Closed

[Core] Improve our import path #2679

oliviertassinari opened this issue Dec 28, 2015 · 20 comments

Comments

@oliviertassinari
Copy link
Member

The tree shaking feature is on his way with webpack 2 (http://www.2ality.com/2015/12/webpack-tree-shaking.html) and will probably soon be implemented by browserify.
I think that, at some point, we will have to deprecate the usage of import:

import ... from 'material-ui/lib/...';

And instead, advice people to use:

import {...} from 'material-ui';
@alitaheri
Copy link
Member

@oliviertassinari How smart and reliable can it really be? How about nested imports? things like svg-icons? how can we address that? I mean we cant put all the icons in the main index file, right? can it actually shake those branches deep down the import tree? I couldn't find info whether it can or not. But there are other issues. Maybe people don't want to enable it for some reasons. I think it's best to support both rather than enforce one over the other.

@newoga
Copy link
Contributor

newoga commented Dec 28, 2015

@oliviertassinari @alitaheri I agree with both of you 😄 .

One of my first challenges with material-ui was knowing where to import components (especially icon components) and I often found myself looking at the source code to find what my import path needed to look like. Putting them all on the root material-ui makes it a lot easier for the user because then you only need to know what the component name is. That makes looking into this webpack feature worthwhile in my opinion.

That being said, being dependent on the reliability of webpack and or browserify is risky and it might also be worthwhile to continue supporting the "full" path for a while.

I'll throw another idea out there that may sound crazy. Material-ui is a big project, and maybe the components deserve their own packages. One possible approach is making material-ui a monorepo. We could put each component in their own directory (like we were planning on doing anyway) and give it its own package.json file with what other dependencies it has (like other material-ui components). That way each component instead of being in a deeply nested path will be flatter with it's own top level name. Like:

import TextField from 'material-ui-text-field'

/* In addition a top level 'material-ui' package could have all components as a dependency and re-export */
import {TextField} from 'material-ui'

Haven't thought through it much. It might be a really bad idea but I thought it's worth mentioning since the React project even follows the monorepo approach.

@alitaheri
Copy link
Member

@newoga Interesting idea. However, the dependency management will be really hard. unlike a library like lodash, this one is very tightly coupled. we'll need to make a huge number of packages (some internal) to include all. besides the svg-icons will prove difficult, they should all be in one package and yet support static imports so only the ones needed get through.

Supporting both styles:

import TextField from 'material-ui/TextField';

and

import {TextField} from 'material-ui';

is easy. but svg-icons will be harder to tackle. but having separate packages for each of them. will be really hard >.> npm will find us and kill us all :D :D

@newoga
Copy link
Contributor

newoga commented Dec 28, 2015

npm will find us and kill us all :D :D

@alitaheri Haha, indeed, they might. I don't want to die. 😆

@alitaheri
Copy link
Member

Me neither xD xD

@oliviertassinari oliviertassinari changed the title Deprecate the import for /lib? [Core] Improve our import path Jan 31, 2016
@mbrookes
Copy link
Member

mbrookes commented Feb 2, 2016

Also related to #1793.

@nathanmarks
Copy link
Member

@alitaheri @newoga this is on the 0.15.0 milestone, where are we going with this right now?

@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

We still haven't discussed and decided on the best approach to implementing this. I think we've settled on putting all the components in directories (along with its internal components and tests, possibly docs).

I think we've also settled on the following path: material-ui/Component

To do this, we're going to have to change our build process to publish the package from within the conceptual lib folder rather than at the root level. To support backwards compatibility, for this version we will likely have to add a lib folder to src to maintain all the old paths and put deprecation warnings in them.

I'm open to better ideas but that's my current thought!

@mbrookes
Copy link
Member

mbrookes commented Mar 8, 2016

To do this, we're going to have to change our build process to publish the package from within the conceptual lib folder rather than at the root level.

It's a shame npm doesn't (appear to) support a way to put the published code in a subdirectory while still referencing it as if it is in the root. This could have been put to good use:

directories.lib
Tell people where the bulk of your library is. Nothing special is done with the lib folder in any way, but it's useful meta info.

@newoga:

To support backwards compatibility, for this version we will likely have to add a lib folder to src

Clever. How will backwards compatibility work for people importing from src? Or is that an inevitable breaking change?

@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

It's a shame npm doesn't (appear to) support a way to put the published code in a subdirectory while still referencing it as if it is in the root. This could have been put to good use:

Yeah it'd be nice if it was supported 😞. I've seen some projects that have a release script that simply copies the package.json to the lib folder then publishes from there. Other projects create a commit and tag where the build is stored on the root with everything else removed and the pubish is still ran from the root. I'm open to either approach.

people importing from src? Or is that an inevitable breaking change?

Yeah, that's kind of what I had in mind. I consider importing from src the same as using internal API and not something we have to enforce backwards compatibility for. I'm hoping we can just use the lib in src trick for the 0.15.0 release, then when we're working on 0.16.0, we can just delete the lib folder.

@mbrookes
Copy link
Member

mbrookes commented Mar 8, 2016

Yeah it'd be nice if it was supported

Open a PR on npm? 😄

@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

Open a PR on npm? 😄

Haha, possibly. Though, I've always got the impression that they try to keep their toolset really simple and allow more sophisticated tooling be developed in user space. Though as far as tooling goes, I think the directories.lib key you mentioned seems like a good use case for this.

@mbrookes
Copy link
Member

mbrookes commented Mar 8, 2016

I'll open an issue and see what they say.

@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

I'll open an issue and see what they say.

Sounds good. 😄

@newoga
Copy link
Contributor

newoga commented Mar 8, 2016

Just another note on this, I know we're already really busy/slammed but I think it would be nice/good form to create a codemod to help users migrate their projects to the new paths.

@oliviertassinari
Copy link
Member Author

this is on the 0.15.0 milestone

Yes indeed, the sooner we do it, the best it will be for everybody.

I think it would be nice/good form to create a codemod to help users migrate their projects to the new paths.

I completely agree and I volunteer to work on this codemod once we have moved the files.

@mbrookes
Copy link
Member

I'll open an issue and see what they say.

I started to write the issue for this the other day, and realised this in't so much an issue for npm as for webpack, browserify, node etc to interpret directories.lib to be an alias for the package root dir. For npm it's just a standards and docs issue.

I started poking through the code of the various tools to see if there's a shared lib used, but didn't have much luck finding anything.

So whilst It would still be nice to have - even if all parties agreed, it won't come soon enough for 0.15.0, and in the unlikely event it did, would force a dependancy on the latest versions of said tools.

tl;dr, we have to do this the hard way.

@nathanmarks
Copy link
Member

@callemall/material-ui

Did we figure out where we are publishing from to make this happen?

Last I recall, we discussed publishing from the build folder.

@oliviertassinari
Copy link
Member Author

@nathanmarks Yeah, let's get this sorted.
I don't think that that name of the folder matters a lot. I would rather focus on the process.
We need a script to copy the components from the src folder to the one we will use to publish.
We also need to filter the unit test components and the examples.

@nathanmarks
Copy link
Member

@oliviertassinari I already added filter out the tests during build when I moved them 👍 (so they don't end up in the lib folder). For the sake of simplicity then, why don't we just have the build process create a minimal package.json and publish from the lib folder?

nathanmarks added a commit to nathanmarks/material-ui that referenced this issue Apr 11, 2016
Improves the import path by creating a package.json
inside the build directory so the library can be
published with a flattened root folder. The lib root
package.json is transformed into a minimal version and
the README, CHANGELOG and LICENSE are copied over.

Resolve: mui#2679
@newoga newoga closed this as completed in 15a57aa Apr 11, 2016
und3fined pushed a commit to und3fined/material-ui that referenced this issue Apr 20, 2016
Improves the import path by creating a package.json
inside the build directory so the library can be
published with a flattened root folder. The lib root
package.json is transformed into a minimal version and
the README, CHANGELOG and LICENSE are copied over.

Resolves: mui#2679
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants