Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Enable resolving generated sandboxes.ts file for rootpath #70

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

Closed
mrmeku wants to merge 1 commit into SoCreate:master from mrmeku:fix-sandboxes-import

Conversation

@mrmeku
Copy link

@mrmeku mrmeku commented Dec 7, 2017

Before if you specified a custom root path angular-playground would be unable to find sandboxes.ts since it required the file to be generated at the root of a workspace. Sandboxes.ts is located using a require statement require(sandboxes) and the esnext module resolution algorithm does not look within the specified root path for the module. To circumvent this, we place sandboxes.ts at the root of the workspace.

Copy link
Contributor

mgmarlow commented Dec 7, 2017

Could you provide more context for this PR? We look for the sourceRoot as a parameter provided in configuration, and place the sandboxes.ts file there by design. Therefore, if you specify your sourceRoot as something other than ./src, that is where the sandboxes.ts file will be placed.

It seems more intuitive for sandboxes.ts to be placed at the sourceRoot instead of the project root, since its function is to export the sandboxes from your application's components, which would also live in sourceRoot.

Copy link
Author

mrmeku commented Dec 7, 2017

So here's the deal. Take a look at this line of code here:
https://github.com/SoCreate/angular-playground/blob/master/src/app/load-sandboxes.ts#L5
. This is how the sandboxes file is loaded into angular playground.
Following the node module resolution algorithm won't find where the
sandboxes.ts file was stored so now webpack will search in places that the
angular cli has configured it to look (namely the root directory and
./src). If you project root is not ./src then webpack will fail to find the
sandboxes.ts file. Thus angular playground currently has an implicit requirement that users with non-default
directory structures must run ng eject and manage their own webpack config which is non-ideal.
My solution to this problem was to place the sandboxes.ts file at the root
of the project so that it could be located regardless of what directory
structure the project has.

Copy link
Contributor

mgmarlow commented Dec 8, 2017

Thanks for describing the problem in more detail. Can you give an example of such a non-standard directory structure? We use Playground internally with a multiple-app structure, e.g. src/app1/app, src/app2/app. Each of these "sub-apps" is outfitted with its own Playground that is configured within a single .angular-cli.json.

As long as the .angular-cli.json root property is consistent between your application and the playground application, I don't see why sandboxes.ts wouldn't resolve properly. For instance, if your non-standard directory root is somepath/other, make sure that both entries in .angular-cli.json have "root": "somepath/other".

Copy link
Author

mrmeku commented Dec 8, 2017 via email

Here's a demo of the bug for you to play with: https://github.com/mrmeku/demo-playground-bug Just clone the repo, do npm install and then try to npm run playground
...
On Thu, Dec 7, 2017 at 5:32 PM Graham Marlow ***@***.***> wrote: Thanks for describing the problem in more detail. Can you give an example of such a non-standard directory structure? We use Playground internally with a multiple-app structure, e.g. src/app1/app, src/app2/app. Each of these "sub-apps" is outfitted with its own Playground that is configured within a single .angular-cli.json. As long as the .angular-cli.json root property is consistent between your application and the playground application, I don't see why sandboxes.ts wouldn't resolve properly. For instance, if your non-standard directory root is somepath/other, make sure that both entries in .angular-cli.json have "root": "somepath/other". — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHteOyce-BWLNSrd5zec1cHEhGh61R3Sks5s-IO5gaJpZM4Q6JjJ> .

Copy link
Contributor

mgmarlow commented Dec 8, 2017
edited
Loading

Thanks for putting that together. I'm going to spend some time looking into this issue--the latest CLI update has caused some problems with multi-app architectures. I've created a ticket on their repo that addresses one such problem, although the error you sent is new to me.

Copy link
Author

mrmeku commented Dec 8, 2017 via email

Thanks for filing that. Lets see where it goes! If they aren't quick to fix it you can always merge this PR as a temporary fix
...
On Fri, Dec 8, 2017 at 11:38 AM Graham Marlow ***@***.***> wrote: Thanks for putting that together. I'm going to spend some time looking into this issue--the latest CLI update has caused some problems with multi-app architectures; I've created a ticket <angular/angular-cli#8809> on their repo that addresses one such problem, although the error you sent is new to me. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHteO5HcgdUffsezt4Od6RV2Sw0w9Qxrks5s-YIzgaJpZM4Q6JjJ> .

Copy link
Contributor

mgmarlow commented Dec 14, 2017
edited
Loading

So I've done a lot of investigation into this issue and I thought I'd provide some findings.

Angular CLI 1.5 update introduces a more strict adherence to the tsconfig.json, according to their release post. Playground uses a project's existing CLI configuration to compile sandbox chunks from the sandboxes.ts file. Therefore, it stands that the increased strictness of tsconfig.json settings along with the issue that I referenced above have led to some issues when using Playground with non-default structures.

Copy link
Author

mrmeku commented Dec 14, 2017 via email
edited
Loading

That makes sense to me, it's more or less what I thought the issue was. The cli is in charge of the webpack config and now the config is more restrictive than it was previously which broke angular playground. The fix within my pull request does play well with the more restrictive webpack config though. With that in mind do you want to merge it? Or do you have another strategy for fixing this bug?
...
On Thu, Dec 14, 2017, 10:58 AM Graham Marlow ***@***.***> wrote: So I've done a lot of investigation into this issue and I thought I'd provide some findings. Angular CLI 1.5 update introduces a more strict adherence to the tsconfig.json, according to their release post <https://blog.angular.io/version-5-0-0-of-angular-now-available-37e414935ced>. Playground uses a project's existing CLI configuration to compile sandbox chunks from the sandboxes.ts file. Therefore, it stands that the increased strictness of tsconfig.json settings along with the issue that I referenced above have led to some issues when using Playground with non-default structures. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHteO4CB5-Cw6uOsLWwJTeCP_kX8-_xcks5tAWG4gaJpZM4Q6JjJ> .

Copy link
Contributor

I am investigating other solutions to this problem that don't involve moving the sandboxes.ts path. I tested it on some other application structures and it didn't resolve the error in all cases.

Copy link
Author

mrmeku commented Dec 14, 2017 via email

One alternative I thought of would be to import it by it's absolute path. You would just need to inject the app root flag into the typescript file which imports the sandboxes. It would be a pretty simple change to make other than exposing the flag
...
On Thu, Dec 14, 2017, 1:32 PM Graham Marlow ***@***.***> wrote: I am investigating other solutions to this problem that don't involve moving the sandboxes.ts path. I tested it on some other application structures and it didn't resolve the error in all cases. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHteO9qx35eLUwAEpGiZP4OBZLRsLCJHks5tAYXmgaJpZM4Q6JjJ> .

Copy link
Contributor

Yeah that is certainly a consideration. That would require modifying the node_modules output at runtime though, since the sandboxes are resolved by webpack before it hits the angular application.

Copy link
Author

mrmeku commented Dec 15, 2017 via email

Yeah, youd have to write a js file to import the path from. Certainly doable, but certainly not as clean as you would hope for ideally
...
On Thu, Dec 14, 2017, 6:11 PM Graham Marlow ***@***.***> wrote: Yeah that is certainly a consideration. That would require modifying the node_modules output at runtime though, since the sandboxes are resolved by webpack before it hits the angular application. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHteO0fHnFuOGbaclSSUSTMv_O6jrkJ0ks5tAccrgaJpZM4Q6JjJ> .

Copy link
Contributor

@mrmeku I've got a PR out that should resolve this issue, #75. I decided not to do use this approach since the latest CLI version (1.6.2) introduced some changes to the webpack configuration that altered the way Playground was chunking out each sandbox individually. #75 fixes both issues.

Copy link
Author

mrmeku commented Dec 27, 2017 via email

Awesome work! I love the solution you chose
...
On Wed, Dec 27, 2017, 12:56 PM Graham Marlow ***@***.***> wrote: @mrmeku <https://github.com/mrmeku> I've got a PR out that should resolve this issue, #75 <#75>. I decided not to do use this approach since the latest CLI version (1.6.2) introduced some changes to the webpack configuration that altered the way Playground was chunking out each sandbox individually. #75 <#75> fixes both issues. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHteO3ll3gYqCATNSk98kmMfFoL2yylOks5tEqEHgaJpZM4Q6JjJ> .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /