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

Production Build for Uploading to App Store #18

Merged
merged 10 commits into from Dec 6, 2018

Conversation

Projects
None yet
2 participants
@littlebee
Contributor

littlebee commented Nov 30, 2018

Description

This PR refactors how the webpack bundle is built for plugins generated.

Instead of building directly into ~/.inivision-studio/plugins/<%= pluginName %>, the webpack output is sent to ./lib/. This is reflected in the plugin manifest entry property for the generated plugin. Additionally, the compiled bundle and the src/ in-editor.(js|tsx) have been renamed index.(js|tsx).

This requires support of the manifest entry property in trapezoid-app. See, InVisionApp/trapezoid-app#4349

To run the generated plugin in the local install of InVision Studio, the generator adds a symbolic link in ~/.invision-studio/plugins at plugin build time (see webpack. I have confirmed fs.symLink works on windows.

The developer experience is the same and the Yeoman prompts are the same as before this PR. Everything still just works.

This also updates the generated manifest file to support the latest spec.

Related JIRA tickets (required)

https://invisionapp.atlassian.net/browse/SPA-265
https://invisionapp.atlassian.net/browse/SPA-266

Verification:

Pull bee/prod-build branch from git

git checkout bee/prod-build

Make sure Yeoman is installed globally and the local directory available to yeoman:

npm install -g yeoman
npm link

switch to another directory and generate a plugin:

cd /tmp
yo

Select Studio plugin from the menu of generators yo presents. You should be able to just hit enter key and accept defaults for all prompts.

cd into the generated plugin dir and build it (tmp-plugin below is the default plugin name):

cd tmp-plugin
npm run build

Plugin should now be available in studio.

@lexicalunit

This comment has been minimized.

Collaborator

lexicalunit commented Dec 3, 2018

A couple things:

  1. I would like to see the symlink creation being a part of the plugin's build process rather than as something that yeoman does. This prevents a number of issues: A) the plugin isn't initially built so having the symlink there presents a problem before that happens. B) If the symlink is deleted by the user, or the user moves their repository, they'd have to re-run yeoman to regenerate it it (or symlink it themselves manually). I think they'd except npm run build to handle that. I would just add an ensure-symlink script to the generated package.json and be sure to call it as part of the build process. Either that or put the symlink creation into the generated webpack configuration.

  2. If the dist/ folder is intended to be the thing that gets packed up and uploaded as the name suggests, we should probably have it be where the symlink points to. We could also copy the manifest.json file into dist/ as part of the build process. Or...

  3. I think perhaps there's a better alternative to using dist/. It seems to be a common practice in many javascript libraries to have both a src/ directory and a lib/ directory, where lib/ is content generated from src/ (for example by babel or some other transpiler). We could keep the symlink as it is now (pointing to the root of the repository). Then change the entry point to be lib/index.js instead of dist/in-editor.js.

@lexicalunit

This comment has been minimized.

Collaborator

lexicalunit commented on generators/app/templates/webpack.config.js in ef8ad15 Dec 4, 2018

Oh good call on this error message! I wouldn't mind also seeing what's being skipped in the log, like, "Skipping plugin reload."

This comment has been minimized.

Contributor

littlebee replied Dec 6, 2018

Good stuff, will add! It would be even more beautiful if we gave them a link to a public facing thing, maybe on developer-portal that detailed what to do to set up, or maybe have a section in the README.md?

This comment has been minimized.

Collaborator

lexicalunit replied Dec 6, 2018

Yeah I really wish we could link them to some public facing docs. Hopefully soon 🤞

@lexicalunit

Added one comment about an error message, not a blocking issue. I still prefer lib over dist, but maybe that's just a personal preference. Great work!

@littlebee

This comment has been minimized.

Contributor

littlebee commented Dec 4, 2018

Hi @lexicalunit, thanks again for the review and insight. Regarding the comments above:

  1. done! Yeah, I went back and forth with which place to put it. Do you think we should move the assertThrowsErrorAsync() and symlinkToStudio() methods out of generated webpack.config.js? Maybe add a ./webpack/helpers.js file with those as exports?

  2. We can change it to lib if you think that is better. I've seen a few projects where 'lib/' or 'out/' is the intermediary build directory - like where .js compiled from .ts goes and 'dist/' is for the bundles. studio upload will bundle everything that is not covered in .studioignore; I was trying to avoid dictating how and where they build things or what paths they use. The upload command will default to grabbing everything in the directory tree up from manifest.json. This strategy should also work with the new require.

  3. okay, let's move it to lib/index.js. I kinda wanted to do that anyway since in-editor.js is no more a thing. Will follow with commit.

@littlebee littlebee requested a review from jasondonnette Dec 5, 2018

@@ -50,7 +111,7 @@ module.exports = (env, argv) => {
{
loader: 'file-loader',
options: {
outputPath: 'assets',
OUTPUT_PATH: 'assets',

This comment has been minimized.

@littlebee

littlebee Dec 6, 2018

Contributor

🐛 I think this was an bad find and replace.

This comment has been minimized.

@lexicalunit

lexicalunit Dec 6, 2018

Collaborator

Ahh oops I missed this. Good catch!

if (!dashboardPort) {
reportError(
'`port` environment variable is not set. Skipping plugin reload. ' +
'Reload plugins manually from the Apps menu in InVision Studio to see changes.\n'

This comment has been minimized.

@lexicalunit

lexicalunit Dec 6, 2018

Collaborator

💯

@lexicalunit

This comment has been minimized.

Collaborator

lexicalunit commented Dec 6, 2018

LGTM 👍

@littlebee littlebee merged commit 861fb80 into master Dec 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@littlebee littlebee deleted the bee/prod-build branch Dec 6, 2018

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