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

Add a public method to know whether the runtime is configured #500

Merged
merged 1 commit into from
Jan 29, 2019

Conversation

stof
Copy link
Member

@stof stof commented Jan 23, 2019

Refs #236

@stof
Copy link
Member Author

stof commented Jan 23, 2019

Failure in the job Highest versions of the dependencies is unrelated to that change. It looks like the hash of the runtime is different for the new version of webpack (they might have altered their runtime a bit).

@stof
Copy link
Member Author

stof commented Jan 23, 2019

Once this is merged and released, we can update the recipe to use it by default.

@stof
Copy link
Member Author

stof commented Jan 23, 2019

and documentation too.

@Kocal
Copy link
Contributor

Kocal commented Jan 23, 2019

Thanks for the feature, it will really helps with PhpStorm webpack integration. 👍

Also I've just seen some linting issues. In the whole file, it use it( instead of it (, and there is always a blank line between it()'s }); and describe()'s });.
I think it would be nice to correct them to keep something consistent. :)

test/index.js Outdated
expect(returnedValue).to.be.true;
});

it ('should return false if the runtime environment has been configured', () => {
Copy link
Member

Choose a reason for hiding this comment

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

has been configured -> has not been configured ?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, bad copy paste

@stof
Copy link
Member Author

stof commented Jan 23, 2019

Also I've just seen some linting issues.

these don't seem to be part of the defined coding standards, as the linting is green on CI. But they are now fixed.

Copy link
Collaborator

@Lyrkan Lyrkan left a comment

Choose a reason for hiding this comment

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

Failure in the job Highest versions of the dependencies is unrelated to that change. It looks like the hash of the runtime is different for the new version of webpack (they might have altered their runtime a bit).

Yeah, that one has been failing for a while, not really an issue.

@stof
Copy link
Member Author

stof commented Jan 23, 2019

should it be marked as an allowed failure then if you don't care about the fact it fail ? That would make the github status more useful.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Jan 23, 2019

I don't know if allowing the job to fail is the right solution... it could be a temporary one though.

What I meant in my other comment is that it's not important if it fails for that specific reason but it could be for another one.

In my opinion the proper way to do it would be to ignore some checks (such as the ones that are based on a hash that we can't control) only when running that job, but I haven't had the time to look into that.

index.js Outdated
@@ -1157,6 +1157,10 @@ class Encore {
return this;
}

isRuntimeConfigured() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Question here: should it be named isRuntimeConfigured or isRuntimeEnvironmentConfigured ? Other methods are taking about RuntimeEnvironment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... now that you point it out I agree that isRuntimeEnvironmentConfigured would probably be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed it.

@Lyrkan Lyrkan merged commit 1219c89 into symfony:master Jan 29, 2019
@Lyrkan
Copy link
Collaborator

Lyrkan commented Jan 29, 2019

Let's merge it that way, thanks @stof

@stof stof deleted the is_runtime_configured branch January 29, 2019 17:09
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Feb 1, 2019
This PR was merged into the 3.4 branch.

Discussion
----------

feat(encore): IDE Integration

This PR add a new entry in the FAQ page of Encore, which explains how to integrate Webpack/Encore in an IDE.

Refs: symfony/webpack-encore#236 & symfony/webpack-encore#500
Ping @Lyrkan

Let me know if I should modify things, thanks.

Commits
-------

d8ca065 feat(encore): ide integration
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.

None yet

4 participants