action-destinations
action-destinations copied to clipboard
segmentio
Handles ES5 transpilation
This PR allows ES5 transpilation. We've recently been using Sentry on our apps and it caught this exception on multiple occasions
SyntaxError: Unexpected token '.' at definition.actionssessionIdperform (webpack://[name]Destination/./src/destinations/amplitude-plugins/sessionId/index.ts:60:23
To fix this issue I suggest to convert the code into ES5 compatible browser.
Testing
Include any additional information about the testing you have completed to ensure your changes behave as expected. For a speedy review, please check any of the tasks you completed below during your testing.
- [ ] Added unit tests for new functionality
- [x] Tested end-to-end using the local server
- [ ] [Segmenters] Tested in the staging environment
@dlasky I'm approving, but the full CI test suite should be run against this branch for a change like this (for example, the tsc build will break if some action tries to use certain features like es6 private fields). How should we handle this?
Sorry to be chiming in on this late (been on vacation the last 2 weeks).
Can you share what browser environment is encountering this error? I'm wondering if it is really necessary to go all the way back to ES5, or if there's a compromise where we can go with ES2015.
I also think we should be handling the transpilation via webpack, not the tsconfig. This would only impact the web bundles then, and would not require cloud mode destinations to run ES5 code. I also think this is safer long-term in case we have dependencies outside of our control that drop ES5 support (assuming that's really needed), and lets cloud mode destinations continue to take advantage of runtime improvements that target newer ES standards.
Sorry to be chiming in on this late (been on vacation the last 2 weeks).
Hi!
Can you share what browser environment is encountering this error? I'm wondering if it is really necessary to go all the way back to ES5, or if there's a compromise where we can go with ES2015.
it happened with Chrome mobile, Safari Mobile, Edge, Firefox, Chrome, Headless Chrome... basically every common browser possible.
I also think we should be handling the transpilation via webpack, not the tsconfig.
To be honest, that is the first thing i tried but i failed in doing so (not very familiar with webpack). But i'll give it another try today.
EDIT : i tried to remove the tsconfig targets and added a target in the unobfuscatedOutput. But i doens't built to es5 this way. I can still see the faulty line in the browser-destinations/dist/destinations/amplitude-plugins/sessionId/index.js
if (context.event.integrations?.All !== false || context.event.integrations['Actions Amplitude'])
which should have transpiled to
if (((_b = context.event.integrations) === null || _b === void 0 ? void 0 : _b.All) !== false || context.event.integrations['Actions Amplitude'])
to work seamlessy with es5 browsers...
@dlasky I'm approving, but the full CI test suite should be run against this branch for a change like this (for example, the tsc build will break if some action tries to use certain features like es6 private fields). How should we handle this?
I'm concerned about how this will impact cloud as well, I would prefer if we figured out the webpack approach since that only affects the bundles built for browser + CDN and not the actual published npm libs which are internally consumed in several other places and have a current expectation of being 2018.
We can of of course test that, but it's a fairly concerning change given the surface area that is potentially impacted.
ok. I think i waas looking at the wrong file trying to see if the transpilation went well.
As i updated webpack config with a target:['web', 'es5'], the dist folder impacted was browser-destinations/dist/web not browser-destinations/dist/destinations/amplitude-plugins/sessionId/index.js.
But now i don't know where to look in the aforementioned folder to check if it transpiled as intended... any clue ?
would like to see a webpack only approach here
Hi Dan, here it is ;) 87bcc3b06c1b986997b4b2deb41a30f5e9b9f160
Yep, i'll approve, but given the nature of the change we'll need to do some fairly extensive testing prior to merging.
Thanks for your patience!
Yes I'll take a look as well. I do think we should be able to get away with transpiling to 2015 instead of es5 - it seems like the issue this is directly fixing is around our use of the nullish operator which isn't supported until later versions of ES.
Yep, i'll approve, but given the nature of the change we'll need to do some fairly extensive testing prior to merging.
Thanks for your patience!
@dlasky Great! thanks
Yes I'll take a look as well. I do think we should be able to get away with transpiling to 2015 instead of es5 - it seems like the issue this is directly fixing is around our use of the nullish operator which isn't supported until later versions of ES.
@chrisradek Do you want me to update the PR ?
Yes I'll take a look as well. I do think we should be able to get away with transpiling to 2015 instead of es5 - it seems like the issue this is directly fixing is around our use of the nullish operator which isn't supported until later versions of ES.
Hi @chrisradek ! i was going to fix my PR and saw in Webpack documentation that
When no information about the target or the environment features is provided, then ES2015 will be used.
(https://webpack.js.org/configuration/target/#:~:text=When%20no%20information%20about%20the%20target%20or%20the%20environment%20features%20is%20provided%2C%20then%20ES2015%20will%20be%20used.)
So i guess that won't fix the issue. Have you had any time to run the extensive tests you had in mind ?
Yes I'll take a look as well. I do think we should be able to get away with transpiling to 2015 instead of es5 - it seems like the issue this is directly fixing is around our use of the nullish operator which isn't supported until later versions of ES.
Hi @chrisradek ! i was going to fix my PR and saw in Webpack documentation that
When no information about the target or the environment features is provided, then ES2015 will be used.
(https://webpack.js.org/configuration/target/#:~:text=When%20no%20information%20about%20the%20target%20or%20the%20environment%20features%20is%20provided%2C%20then%20ES2015%20will%20be%20used.)
So i guess that won't fix the issue. Have you had any time to run the extensive tests you had in mind ?
Hi @dlasky , @chrisradek ! How are you doing ? Do you guys have had time to look at that PR and messages here ? Best
Hi @dlasky , @chrisradek ! How are you doing ? Do you guys have had time to look at that PR and messages here ? Best
Hi @dlasky , @chrisradek ! Could you tell me please if there's anything else i can do ?
Is this going to be merged ? We missed events from 16k users because of this. We are thinking to move to server-side calls instead to avoid problems :/
@chrisradek This would only impact the web bundles then, and would not require cloud mode destinations to run ES5 code. Since only browser-destinations/tsconfig.json, it shouldn't affect cloud mode destinations?
~~Can we just go back to the fix of making browser-destinations/tsconfig.json target ES2015?~~
@chrisradek This would only impact the web bundles then, and would not require cloud mode destinations to run ES5 code. Since only browser-destinations/tsconfig.json, it shouldn't affect cloud mode destinations?
Can we just go back to the fix of making browser-destinations/tsconfig.json target ES2015?
Can we ?
@massinissanm Ahh, I remember why we prefer to use webpack -- there are shared / core dependencies that we also want to bundle, and just changing the target of this one repo won't do that. We don't want to have to change the tsconfig build targets of those shared dependencies -- since they are also run in node.js. We want to bundle and transpile everything together and we need webpack for that.
Hey all, we're still having issues that this PR was hoping to resolve. Things look like they have stalled with this PR, is it possible to get it across the finish line or is another approach required?
