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

Pre-compiling metal imports #109

Closed
bryceosterhaus opened this issue May 11, 2016 · 32 comments
Closed

Pre-compiling metal imports #109

bryceosterhaus opened this issue May 11, 2016 · 32 comments
Assignees

Comments

@bryceosterhaus
Copy link
Member

We noticed when transpiling a component that metal imports are expecting us to transpile them rather than already being built.

I noticed that all the React imports we use right now are built when they publish the module.

By default we exclude node_modules from our build process because when we include them, the build process takes far longer.

Is there a reason for this, or what are the exact thoughts on this?

@yuchi
Copy link
Contributor

yuchi commented May 12, 2016

Are you requiring/importing them through import x from 'metal/src/x'?

@mairatma
Copy link
Contributor

You're right, that's the default behavior for Metal.js modules right now. The reason why we're doing this is that we want to support any kind of build format (AMD, commonjs, globals), so we keep the original file so you can transpile to whatever you wish.

That said, I agree that it takes longer and it would be nice to precompile. Since you're saying that react precompile their imports I'm going to take a look at what they build it to and see if we can do something similar, or at least have both options.

@mairatma mairatma self-assigned this May 12, 2016
@yuchi
Copy link
Contributor

yuchi commented May 12, 2016

Sorry, but as far as I see metal is distributed as compiled too.

What’s actually the matter here?

@yuchi
Copy link
Contributor

yuchi commented May 12, 2016

Ok, you’re talking about components (such as this)

@mairatma
Copy link
Contributor

Yeah, like @yuchi said, we do provide the source code compiled to commonjs, but that's for modules that would be able to be used in node environments.

Not sure if always compiling everything to commonjs would help for components, probably umd would make more sense or something like that. I'll check what react and others do to get an idea.

@yuchi
Copy link
Contributor

yuchi commented May 12, 2016

@mairatma what they do is incredibly straightforward:

  • publish the sources in whatever format/language typically in the src/ folder;
  • publish the compiled Synchronous CommonJS modules typically in the lib/ folder;
  • correctly point the package.json’s main to the compiled file;
  • publish a bundled AMD (actually UMD) file typically in the dist/.

This is the main reason I don’t really understand how Lfieray can go with AMD alone.

@yuchi
Copy link
Contributor

yuchi commented May 12, 2016

@mthadley
Copy link

mthadley commented May 12, 2016

In @bryceosterhaus's case, he was doing something like this while excluding node_modules from his webpack config:

import Component from 'metal-jsx';

class MyComponent extends Component { ... }

but inspecting the package.json and it's main, that module has not been pre-compiled.

@mairatma
Copy link
Contributor

@yuchi, that makes sense, UMD can be more easily used in projects with different module dependencies, so I can see why they expose this format as well.

@mthadley as I said we do this by default to allow different types of build outputs. Btw, which tool are you guys using to build metal components? Are you using our tools (gulp-metal or metal-cli), or external ones (like webpack)? Just curious to understand more the use case so I can try to think of a way to help with it. I agree that it'd be much better to have a way to use pre compiled dependencies by default.

@bryceosterhaus
Copy link
Member Author

@mairatma, we are using webpack to build all of our components.

@mairatma
Copy link
Contributor

I see, it seems like it can handle commonjs modules as well, so in our case we'd probably just need to always publish the compiled commonjs files in liband point main to it, like we do for node modules.

I'll verify that this would work as expected and apply it to our projects in case it does :)

@bryceosterhaus
Copy link
Member Author

That would be great. Let us know if we can help at all.

Thanks Maira!

@yuchi
Copy link
Contributor

yuchi commented May 12, 2016

@mairatma actually UMD is not the right one to use. In fact you cannot access directly the modules that make up the whole thing. It is incredibly common in react land, for example, to use stuff that is in react/lib/… if you just need that. Also, this lets plugins and components use only part of a bigger library without burdening the consumer.

@mairatma
Copy link
Contributor

Sure, it's definitely better to use the modules from the lib folder, it's just that I understand that this other option could be useful in some use cases, where you don't want to set up any module loading at all, so I see why they'd do that.

But I also agree that it's more important to have the commonjs build on the lib folder. Let's focus on that, we can do another option like UMD later if we're convinced it's worth it as well :)

@mairatma
Copy link
Contributor

@bryceosterhaus thanks! Help will certainly be appreciated. Once I confirm that this is the way to go, we'll need update each repo that is not doing this yet, so it would great if we could divide the efforts there.

I'm currently busy working on some documentation, but as soon as I have some time I'll go into this and update you on this issue, so we can start the convertions :)

@mairatma
Copy link
Contributor

I've just confirmed that compiling all our repos to lib will allow building just the project's src files when using either browserify or webpack, without having to build Metal.js files as well.

In these cases we should stop using the metal preset and only use the es2015 preset directly instead, since metal's preset only adds es2015 and transforms import calls to point to the es6 files, which won't be needed when using these build tools :)

So we "just" need to update all the metal repos to compile to lib when published. I was going to try to divide the efforts for this, but I've just realized that I'll need to go to each repo to publish new versions for them anyway, so dividing this won't help much.

It's just repetitive work, but they're all small changes, so I'll focus on this tomorrow and I'll let you guys know when it's ready so you can update your builds to work with the new precompiled files :)

@mthadley
Copy link

Awesome! Thanks @mairatma. That will definitely help to simplify our build.

@yuchi
Copy link
Contributor

yuchi commented May 16, 2016

@mairatma, fantastic news. May I suggest you guys to switch to a multi-package repository? See Lerna for a tool that helps you on this.

@mairatma
Copy link
Contributor

Yeah, I've thought about that a few times. The only thing I don't like so much about that approach is that the commits for all the packages end up in the same repo, while keeping them separate keeps the commit history for each package really separate as well, easier to go through.

That said I can see how switching and using Lerna would really help on this and other cases where we'd like to change the configuration of all packages and publish them. And it's always possible to see partial history for just the package you wish anyway. I'll talk to Eduardo about this, I think it would also be less confusing for developers wanting to report issues for example. Thanks for the suggestion :)

@yuchi
Copy link
Contributor

yuchi commented May 17, 2016

I do understand your concerns and I’m still troubled too on this.

mairatma added a commit to mairatma/senna.js that referenced this issue May 17, 2016
Right now users are forced to always compile senna.js code themselves when using it through es6 imports. It's common practice to provide the precompiled code in commonjs in the "lib" folder, so that developers using build tools like "browserify" and "webpack" can worry just on compiling their own code, instead of having to do it for dependencies as well.

See metal/metal.js#109 for more details.
@mairatma
Copy link
Contributor

I've hunted all metal core modules, as well as any of their dependencies (like senna.js) which had the same problem, and published a new version of all of them with precompiled code.

I tried it out on a demo that I had built with metal-jsx before (check it out here), and successfully managed to tell webpack to stop compiling anything in node_modules.

So I believe that you'll already be able to use this as you wanted @bryceosterhaus. Just make sure that all dependencies are at their latest version. Let me know if you find any module I may have missed that still needs to precompile code.

The only ones I know I didn't work on yet are the soy components, which you probably won't need since you're using JSX. I'll do the same for them, but later, when reorganizing them (maybe into a single metal-components repo with lerna, like @yuchi suggested). But if they're also needed now I'll convert them too.

@bryceosterhaus
Copy link
Member Author

Should we also do this for components like metal-modal? I noticed that it did not get compiled into a lib folder

@mairatma
Copy link
Contributor

Yes, like I said above, I didn't do this for the soy components yet (like metal-modal) since it wasn't urgent and there are many things to be done. But this will be done, which is why I haven't closed this issue yet :)

@bryceosterhaus
Copy link
Member Author

Oh gotcha, that totally makes sense, I must've miss read that above. Thanks!

@bryceosterhaus
Copy link
Member Author

I just tried to add pre-compiling to metal-modal and ran into an issue where the built Modal.soy.js imports form the src folder explicitly.

import Component from 'metal-component/src/Component';
import Soy from 'metal-soy/src/Soy';

Is it possible to change those as well? Otherwise, we can't exclude node_modules in webpack.

@mairatma
Copy link
Contributor

Hmm I remember there was a reason why we were doing this on the soy file, related to how liferay was using the compiled files, but I don't remember right now. I'll try to find out and see a better solution so we can have this.

@bryceosterhaus
Copy link
Member Author

That would be great. No rush on it, just thought I would document it now since I just saw it.

Thanks @mairatma!

@yuchi
Copy link
Contributor

yuchi commented Jun 14, 2016

In Liferay the libraries are scanned for *.es.js and compiled to AMD modules.

Importing from src means you were leveraging the standard build compilation and resolution, also transforming them from CommonJS to AMD.

@mairatma
Copy link
Contributor

That makes sense, I imagined it was something related to how the js is built. I'll see what we can do to make this work on liferay while not disabling us from precompiling our code.

@mthadley
Copy link

Hey guys, just wanted to jump in and see what the status of this is. We were specifically hoping to use metal-modal, but if there is no time, we can always write our own JSX modal.

@mairatma
Copy link
Contributor

Hey @mthadley, you should already be able to use metal-modal with version rc.8 actually :)

I've published a new version of gulp-metal that stops using full paths for soy compiled files, but that doesn't affect metal-cli which is used by Liferay yet. So I managed to pre-compile metal-modal without affecting Liferay immediately.

I haven't published the other components with pre compiled files yet, I'm working on something else but I'll go back to this immediately after I finish that.

@mairatma
Copy link
Contributor

I've finally finished making all metal components publish pre-compiled code (hopefully), so closing this :)

Let me know if any of them doesn't work as they should with the pre-compiled code, or if I missed any by mistake.

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

4 participants