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

[WIP] Setup angular template for embark #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iamonuwa
Copy link

@iamonuwa iamonuwa commented Sep 15, 2018

@iurimatias please review

Fixes #1

@iurimatias iurimatias requested a review from a team September 15, 2018 17:16
Copy link
Contributor

@iurimatias iurimatias left a comment

Choose a reason for hiding this comment

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

Hi @iamonuwa

I did the following and got an error:

embark new angulartest --template iamonuwa/embark-angular-template
cd angulartest
embark run --nodashboard

Error:

./app/main.ts
Module not found: Error: Can't resolve './app/app.module' in '/Users/iurimatias/Projects/tmp/test3/angulartest/app'
resolve './app/app.module' in '/Users/iurimatias/Projects/tmp/test3/angulartest/app'
  using description file: /Users/iurimatias/Projects/tmp/test3/angulartest/package.json (relative path: ./app)
    Field 'browser' doesn't contain a valid alias configuration
    using description file: /Users/iurimatias/Projects/tmp/test3/angulartest/package.json (relative path: ./app/app/app.module)
      no extension
        Field 'browser' doesn't contain a valid alias configuration
        /Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module doesn't exist
      .wasm
        Field 'browser' doesn't contain a valid alias configuration
        /Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module.wasm doesn't exist
      .mjs
        Field 'browser' doesn't contain a valid alias configuration
        /Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module.mjs doesn't exist
      .js
        Field 'browser' doesn't contain a valid alias configuration
        /Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module.js doesn't exist
      .json
        Field 'browser' doesn't contain a valid alias configuration
        /Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module.json doesn't exist
      as directory
        /Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module doesn't exist
[/Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module]
[/Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module.wasm]
[/Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module.mjs]
[/Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module.js]
[/Users/iurimatias/Projects/tmp/test3/angulartest/app/app/app.module.json]
 @ ./app/main.ts 3:0-45 10:41-50
 @ multi ./app/main.ts
./app/main.ts
Module not found: Error: Can't resolve './environments/environment' in '/Users/iurimatias/Projects/tmp/test3/angulartest/app'
resolve './environments/environment' in '/Users/iurimatias/Projects/tmp/test3/angulartest/app'
  using description file: /Users/iurimatias/Projects/tmp/test3/angulartest/package.json (relative path: ./app)
    Field 'browser' doesn't contain a valid alias configuration
    using description file: /Users/iurimatias/Projects/tmp/test3/angulartest/package.json (relative path: ./app/environments/environment)

@iamonuwa
Copy link
Author

@iurimatias thank you for the review. I'll fix this issue

@0x-r4bbit
Copy link

0x-r4bbit commented Sep 20, 2018

@iamonuwa @iurimatias just as an FYI:

Turns out this is also an issue in embark-typescript-template. I'm not very experienced with Webpack but this may be related to the template using babel-loader instead of ts-loader.

@iamonuwa let us know how it goes!

@0x-r4bbit
Copy link

For reference, I created embarklabs/embark-typescript-template#2

@iamonuwa if you get this fixed, you eliminate two issues in one go! :)

@iamonuwa
Copy link
Author

@PascalPrecht can we use gitter to chat? Shoot me a message there. @iamonuwa

@iamonuwa
Copy link
Author

I'll use babel-loader inside the webpack.config.js to work on this.

@0x-r4bbit
Copy link

0x-r4bbit commented Sep 20, 2018

@iamonuwa sorry it's late here so I'll have to keep it short. Could you actually reproduce the issue or did the compilation work for you?

We noticed that this may only occur in embark versions <= 3.2.x, so if you're working with the latest stable release, it might be possible those errors don't show up for you.

The reason for that is because in 3.2.x, embark uses the webpack config from your dapp as supposed to its internal one. However if it does work with 3.1.x (I'm gonna verify this tomorrow morning), then this could be a good pointer for where to start looking!

@iamonuwa
Copy link
Author

@PascalPrecht you are right. Am finding it difficult to reproduce the error on 3.1.x

@iamonuwa
Copy link
Author

@iurimatias this is my terminal embark version 3.2.0-develop

image
Cannot reproduce the error as well.

@0x-r4bbit
Copy link

Oh that's a different error indeed. Can you remove .embark/ebark.ipc and try again?

@iamonuwa
Copy link
Author

@PascalPrecht I don't have .embark directory

@0x-r4bbit
Copy link

0x-r4bbit commented Sep 21, 2018

Hm.. your output says though that it listens to angulartest/.embark/embark.ipc

Can you verify once more please?

@iamonuwa
Copy link
Author

@PascalPrecht just saw that, was looking at the my computer root directory

@0x-r4bbit
Copy link

Yes, sorry for not being too clear about that!

@iamonuwa
Copy link
Author

Yeah, I guess the fault should be from me.
I got a new computer. Need to add ipfs first and try again

@iamonuwa
Copy link
Author

@PascalPrecht still same error. I cannot reproduce @iurimatias error locally

@0x-r4bbit
Copy link

Okay, this is a different thing then. Can you scaffold a dapp newly with your template and embark being on 3.2.0-develop?

@iamonuwa
Copy link
Author

@PascalPrecht, if I get you correctly, you need me to setup a new dApp with the requirements specified on the bounty?

@0x-r4bbit
Copy link

Oh sorry, what I meant was doing the exact steps that @iurimatias did here #2 (review)

Remember that you've done your work on 3.1.x so to reproduce this it'd help to scaffold a new app using your template while using embark 3.2.x.

Does that help?

@0x-r4bbit
Copy link

I've also just noticed that 3.1.10 (latest official release) doesn't actually come with a --template option yet. So this is a 3.2.x feature and should probably be implemented based on 3.2.x

@iamonuwa
Copy link
Author

@PascalPrecht ATM, I still can't reproduce that error.

@iurimatias
Copy link
Contributor

@iamonuwa I was able to get past the error I reported by using .ts for e.g import { AppModule } from './app/app.module.ts';

then it breaks with this instead, which is something that needs changes to the template as part of the PR:

./app/app/app.module.ts
Module build failed (from /Users/iurimatias/Projects/Status/embark/node_modules/babel-loader/lib/index.js):
SyntaxError: /Users/iurimatias/Projects/Status/tmp2/AngularTemplate/app/app/app.module.ts: Support for the experimental syntax 'decorators-legacy' isn't currently enabled (6:1):

  4 | import { AppComponent } from './app.component';
  5 |
> 6 | @NgModule({
    | ^
  7 |   imports: [
```

The screenshot you put above, seems to indicate you don't `geth` installed, is that the case?

@iamonuwa
Copy link
Author

@iurimatias I'll look at it again

@0x-r4bbit
Copy link

@iamonuwa this option should get you there! https://www.typescriptlang.org/docs/handbook/decorators.html

:)

@michaelsbradleyjr
Copy link

The module imports problem can likely be fixed by implementing the same changes to webpack.config.js as made for the typescript template:

embarklabs/embark-typescript-template#3

@iamonuwa
Copy link
Author

@PascalPrecht @iurimatias please review

Copy link

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

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

Hey @iamonuwa

thanks for the update and we're getting there! I ran your branch locally and it turns out that stuff is still in flux. Did you actually tried running this to see whether things are working?

Here are some pointers:

  • @babel/plugin-proposal-decorators is missing as a module, making it impossble to run embark run
  • The patch applied to base.resolve.extensions needs to happen earlier in the config algorithm. If you try running the template the way it is right now, you'll notice that we still run into module resolution errors
  • Having all the above fixed, the code will compile, however at runtime it doesn't look as bright yet. It seems like Angular isn't bootstrapping. Again, please verify that you changes are working :)

Apart from that I left some minor comments inline for help and guidance.

import { environment } from './environments/environment';
if (environment.production) {
enableProdMode();
}

Choose a reason for hiding this comment

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

It looks like we have some wrong indentation here.

Copy link
Author

Choose a reason for hiding this comment

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

LGTM

Choose a reason for hiding this comment

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

Sorry, I'm not sure I follow.. but maybe I wasn't clear enough. I wanted to point out that there's some wrong formatting, the } being indented, which probably shouldn't be.

@@ -0,0 +1,3 @@
import '@angular/platform-browser';
import '@angular/platform-browser-dynamic';
import '@angular/core';

Choose a reason for hiding this comment

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

I believe this file isn't needed. It doesn't seem to be imported anywhere. Can you verify this @iamonuwa ?

]
],
[
'@babel/plugin-proposal-decorators', {

Choose a reason for hiding this comment

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

Running this code will result in an error that @babel/plugin-proposal-decorators is missing.

This needs to be added to package.json

'.wasm', '.mjs', '.js', '.json',
// typescript extensions
'.ts', '.tsx'
];

Choose a reason for hiding this comment

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

Defining those extensions here actually won't take any effect as base is being used before it gets extended here. Please ensure it's base is in shape before it's used for creating dev and prod environments.

@iurimatias
Copy link
Contributor

@iamonuwa any updates? we're looking forward for this template

@iamonuwa
Copy link
Author

@iurimatias I can't reproduce this error. Seems like I'll stop work if its urgent

@0x-r4bbit
Copy link

@iamonuwa not urgent! :)

But we'd like to get your changes in. Unfortunately, we need to make sure that everything works first. Did you test this template locally and got a working app running with this code?

If yes, can you provide steps on how to reproduce this? Because I'm unable to make this work.

Thanks @iamonuwa !

@iamonuwa
Copy link
Author

Great @PascalPrecht. I'll do a video recording today

@0x-r4bbit
Copy link

@iamonuwa Looking forward to it!

@frankchen07
Copy link

@iamonuwa - Frank from Gitcoin here, is there any followup on this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants