Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Get ready to publish Alloy as an NPM repo #637
Get ready to publish Alloy as an NPM repo #637
Changes from 8 commits
5548e15
8cdcc34
fdf619f
ca0139e
f4f37ba
6213606
6cc1416
9b459e2
c48cc80
ca0892a
8e34421
41fcfda
821977d
ccbbef9
922bb03
a2a9893
dda91a0
a46c2d4
3ce8902
f4c5646
d54c472
0b5b65d
eb9e8f7
52e5fee
d18dc14
f5f7c57
a5e7a39
ff5b019
b8eb968
f008c43
55d7cd3
acf533f
345ba85
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically we should change "Adobe Launch" to "Adobe Experience Platform Launch" and remove "the" from before "Adobe Experience Platform Web SDK". We should probably lowercase "Extension" because "Adobe Experience Platform Web SDK Extension" isn't a brand itself and "Extension" alone isn't a pronoun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"launch" here is a secondary reference so it should probably be "Experience Platform Launch" (my understanding is we can can drop the "Adobe" on secondary references).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lowercase "Extension" and change "launch" to "Experience Platform Launch".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"standalone" is one word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"rollup system" should probably be "bundler" or "JavaScript bundler".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions here:
(1) Are you aware of any bundlers that don't support
module
by default? I'm aware it isn't standard, but I think it's been supported by default for 2+ years on the major bundlers. If so, this line may not be providing value and just introducing cognitive load, but who knows. If you keep it, it might be better to say something like "are exposed by the NPM package via themodule
field of its package.json, so you may...". If I'm a consumer having problems, that might give me more context for googling. 🤷♂️(2) I'm trying to think through why we're creating
main.js
andmodules.js
as bundles. Normally what I've seen is that you would just run all the source code files through babel and dump all the resulting files into, say, anes5
directory, then point themain
field in package.json toes5/index.js
. For the es6 variant, you may just be able to point themodule
field in package.json tosrc/index.js
.One of the benefits of not generating bundles for the npm package consumers is that if the consuming application already has a dependency that matches one of our dependencies, the final application bundle only needs to include the dependency code once. Currently, we only have a couple dependencies, so that chance is somewhat low, but that may grow over time
One effect of taking this approach is that the extension will need to do some bundling before getting packaged and uploaded. But, I believe this would mean that you would be able to move the Launch core module dependencies from
peerDependencies
back todependencies
in this repo. When you do the bundling for the extension, I believe you'd just mark Launch core module dependencies as "externals" so that Launch can provide them instead of bundling them in fromnode_modules
.Sorry, I should've thought through this during my previous review as it definitely affects what I was saying in https://github.com/adobe/alloy/pull/632/files#r518529523.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think all this documentation should be consolidated into the user docs like I mentioned in #632 (comment). It sounds like you might disagree, but you did mention you were considering at least trimming down this readme to the es6 installation instructions.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that pull typescript into the library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I didn't mean to add this as a dependency. Thanks for catching.