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

include runtime dependencies for emmet #22

Merged
merged 1 commit into from
Mar 19, 2020
Merged

Conversation

marcdumais-work
Copy link
Contributor

Fixes #18

As suggested here, I added one step to install the production dependencies for emmet: #18 (comment)

It seems to fix #18 and as well make the emmet built-in work: see second video here: #18 (comment)

How to test

Build this branch using yarn, then test the emmet built-in as per the following comment, and verify that this scenario no longer generates an exception: #18 (comment)

@vince-fugnitto
Copy link
Member

@marcdumais-work would it be harmful to do the same thing for all extensions as to not have any potential runtime dependencies issues?

@vince-fugnitto
Copy link
Member

I'm fine as well with doing the update on a case-per-case basis as well if that's the strategy.

@akosyakov
Copy link
Member

@vince-fugnitto we could check whether package.json has dependencies field and only then run the script

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@marcdumais-work I verified the changes and I now see emmet snippets being successfully contributed and suggested when editing html files. I also no longer see the frontend error reported.

@marcdumais-work
Copy link
Contributor Author

would it be harmful to do the same thing for all extensions as to not have any potential runtime dependencies issues?

I do not think it would be harmful, and Anton's suggestion of narrowing it down to extensions that have runtime dependencies could be even better. However I am hesitant to do this right now since I do not have much time to properly test all affected built-ins.

I've opened an issue instead for now: #23

@marcdumais-work
Copy link
Contributor Author

@akosyakov are you fine with me merging this PR?

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

fine :)

@marcdumais-work marcdumais-work merged commit 2997b3e into master Mar 19, 2020
@marcdumais-work marcdumais-work deleted the fix-emmet branch March 19, 2020 19:54
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.

emmet: extra packaging steps may be required
3 participants