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

Fix Missing Build Tools Dependency + Postinstall Fix #1035

Merged
merged 6 commits into from
Jan 15, 2019

Conversation

sghoweri
Copy link
Contributor

@sghoweri sghoweri commented Jan 14, 2019

JIRA

N/A

Summary

Addresses two dependency-related items flagged with the initial Bolt v2.3.0 release candidate.

Note: this will also be included in the next v2.3.0 release candidate: v2.3.0-rc.1

Description

  1. Adds the missing Twig Renderer dependency to @bolt/build-tools
  2. Updates the way PHP-related dependencies are installed. This update removes using the NPM postinstall script so certain dependencies aren’t attempted to be auto-installed in Drupal or any downstream consumer — only in the Bolt monorepo automatically via the Lerna exec command.

Testing Instructions

  • Verify everything installs and compiled as expected in Bolt
  • Verify these updates sufficiently address the initial issues flagged in Drupal. @remydenton any thoughts on how we can cleanly double check this without having to do another full release?

@remydenton
Copy link
Collaborator

🤔 I can't think of a good way to fully test without doing any publishing (is there a way to pull in npm packages that aren't published at all, such as by specifying a local path to a package? I don't know of one).

We could potentially do some canary releases, though I'm not sure that would prevent any more headaches than just doing a point release and hoping for the best...

@sghoweri
Copy link
Contributor Author

sghoweri commented Jan 14, 2019

🤔 I can't think of a good way to fully test without doing any publishing (is there a way to pull in npm packages that aren't published at all, such as by specifying a local path to a package? I don't know of one).

We could potentially do some canary releases, though I'm not sure that would prevent any more headaches than just doing a point release and hoping for the best...

@remydenton

Yeah I was considering either us doing a canary release with just the packages updated here OR mentioning that you could reference NPM packages locally if you have the repo checked out (ex. check out https://github.com/bolt-design-system/bolt/blob/master/docs-site/package.json#L61).

I think the tricky part of this is that the changes involve dependencies of dependencies which, while you could technically try to wire up locally, would quickly get out of sync with the actual changes expected to get published.

I'm leaning a little bit more towards doing a canary release on this but could be swayed if @margoromo or @danielamorse had any thoughts on this

Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

Approved. Did a fresh install locally, ran start and build, and all worked as expected.

@sghoweri not sure I can add much to the canary vs point release conversation. I like the idea of a canary release better so that we don't potentially have another broken release in the wild, but I don't actually know how we make a Bolt canary release. I've only seen the point release process.

Copy link
Collaborator

@remydenton remydenton left a comment

Choose a reason for hiding this comment

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

Though I'm not able to actually test in Drupal without a canary release or similar, this all makes sense from a code review perspective. It should, theoretically, resolve both the issues I saw in Drupal.

package.json Show resolved Hide resolved
@sghoweri sghoweri modified the milestones: v2.3.0-rc.1, v2.4.0 Jan 15, 2019
@sghoweri sghoweri merged commit 8b16409 into master Jan 15, 2019
@sghoweri sghoweri deleted the fix/twig-renderer-fix branch January 15, 2019 13:47
This was referenced Feb 1, 2019
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.

3 participants