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

Allow users to bundle node_modules #580

Merged
merged 7 commits into from
Sep 14, 2022

Conversation

lucasfcosta
Copy link
Contributor

Please notice that I had to rebase this PR on top of #576 so that we can actually parse streaming responses from Kibana.

Summary

This PR allows users to bundle modules into the scripts they push.

I've marked it as draft as it still needs E2E tests.

How to test this PR

To test this PR, try including a JS library into your journey, by importing it into the journey file.

Then push your journey with the extra imported module to Kibana.

You should be able to run the Journey just fine.

@lucasfcosta lucasfcosta marked this pull request as draft August 17, 2022 17:22
@apmmachine
Copy link

apmmachine commented Aug 17, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-14T18:13:51.777+0000

  • Duration: 14 min 34 sec

Test stats 🧪

Test Results
Failed 0
Passed 185
Skipped 2
Total 187

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

// Spin off another build to copy over the imported modules without bundling
const result = await esbuild.build({
...commonOptions(),
entryPoints: {
[entryPath]: entryPath,
},
bundle: false,
bundle: true,
Copy link
Member

Choose a reason for hiding this comment

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

this would change our journey files format IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's interesting, do you mean the file format itself (the extension)? Or do you mean the content format (i.e. bundling the CommonJS infra)?

In my tests, I added an fs.writeFile to the code so I could save the generated zips to my disk. Then, once I opened them, I did manage to see that the file extension was .js and that the content was also valid JavaScript.

I also checked the docs here: https://esbuild.github.io/api/#bundle and couldn't see a reference for format changes.

Can you go into a bit more detail about your concern here? Perhaps I'm missing something or misunderstood.

Copy link
Member

Choose a reason for hiding this comment

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

I mean the content of the journey itself. Lets say when you push a journey file file.journey.ts, What does the content look like? Did it get changed to something else and whether the files were bundled - concatenated in to a single file for the files(utils, helpers) that were imported by the journey?

@lucasfcosta lucasfcosta marked this pull request as ready for review August 18, 2022 10:30
@lucasfcosta lucasfcosta force-pushed the bundle-node-modules branch 4 times, most recently from a9136ce to e0f58e5 Compare August 22, 2022 14:01
@lucasfcosta
Copy link
Contributor Author

CC @vigneshshanmugam feel free to merge this whenever you're happy with it, all tests are now passing 😄

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Even though this solution works, this defeats the purpose of our Plugin and kind of bundles everything together in a single file.

The goal of our push command is to replicate exactly how your folder structure is on your local and run the same inside HB when pushed. This would make it presentable in Kibana when we add unzipping support and allow users to view and edit monitors.

TLDR: If i push the advanced.journey.ts to Kibana, the structure should like

// Main branch
/example/todos/advanced.journey.ts
/example/todos/helper.ts


// PR branch
/example/todos/advanced.journey.ts

@lucasfcosta
Copy link
Contributor Author

@vigneshshanmugam can you double check whether you had the last version during your test? That behaviour you mentioned was happening before the changes I did yesterday morning.

Right now, I just tried to push a journey which depends on another local file and it did separate the necessary files.

To test that, I added the following line to src/push/bundler.js in the build function to get the ZIP file:

  async build(entry: string, output: string) {
    await this.prepare(entry);
    await this.zip(output);
    const data = await this.encode(output);
    await writeFile('/tmp/new-synth.zip', data, { encoding: 'base64' });
    await this.cleanup(output);
    return data;
  }

Then I extracted the new-synth zip file and got two separate files:

Screenshot 2022-08-24 at 13 19 45

Is this what you meant? Or did I misunderstand?

@paulb-elastic
Copy link
Contributor

As discussed in Tech Sync, we should ensure this feature includes a check and handling the failure of the compressed bundle being too big for either the Saved Object, or configmap to support, and feed this back to the user (e.g. ...failed to create the monitor as the bundle size is bigger than the maximum 800KB...)
cc @lucasfcosta

@lucasfcosta
Copy link
Contributor Author

@vigneshshanmugam I was looking into not bundling the node_modules but in that case we'd have to implement the resolution ourselves as the plugin we use bundles them by default and it doesn't seem like we can change that.

Considering that Kibana only shows the code inside the step, as shown in the screenshot below, what do you think of us merging this one as is?

Screenshot 2022-08-31 at 10 35 28

CHANGELOG.md Outdated Show resolved Hide resolved
@StuGray86
Copy link

Hi guys.

We're currently waiting on this change, is there any timeframe for its release?

@lucasfcosta
Copy link
Contributor Author

Hey @StuGray86, we're trying to expedite it at the moment, so we're working actively on it. I can't promise exact dates, but we'll do our best for it to be speedy.

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@awahab07
Copy link

awahab07 commented Oct 4, 2022

Post FF Testing

I was able to import a simple library like uuid and successfully create, push and run the journey.

Script I used:

import { journey, step, monitor } from '@elastic/synthetics';

import { v4 as uuidv4 } from 'uuid';

journey('Journey uuid Test', ({ page, params }) => {
  monitor.use({
    id: 'uuid-lib-test-001',
    enabled: true,
    schedule: 20,
  });

  step('Generate UUID', async () => {
    const uuid = uuidv4();
    await page.goto('https://onlinetexttools.com/convert-text-to-image');
    const textAreaLocator = page.locator('textarea').first();
    await textAreaLocator.fill(uuid);
    await textAreaLocator.type(' ');
  });
});

image

However, I did find an issue with bundling certain imports and raised a bug.

@awahab07 awahab07 removed their assignment Oct 4, 2022
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.

6 participants