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

Add --webextension option to command line #2

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

Andras-Simon
Copy link

The --webextension option will automatically add webextension-polyfill to your extension by default as discussed in #1

Please don't merge this PR yet.

The --webextension option will automatically add webextension-polyfill to your extension by default.
@dutiyesh
Copy link
Owner

dutiyesh commented Oct 19, 2019

As most of the users would want their extension to work cross-browser, I was thinking of keeping WebExtension as a built-in support and not enable it through an option from command line.

What do you think?

@Andras-Simon
Copy link
Author

Good point. However if WebExtensions will be enabled by default that will break backward compatibility (eg. if someone already used the cli to generate a project and uses it again the new version the outputs will differ). It might be not a big deal, just a thing to consider.

@dutiyesh
Copy link
Owner

I don't think this change will break existing code since we are adding a new feature and not replacing.

And while releasing it we are going to mention it in our changelog file, so that user can know the version from which WebExtensions support was introduced.

@Andras-Simon
Copy link
Author

Alright @dutiyesh ! Should I then change it to something like --no-webextension in case somebody wants to build an extension without Webextension support?

@dutiyesh
Copy link
Owner

How about --no-cross-browser?
It is a generic term and hides the implementation details (WebExtension API) behind disabling cross-browser support.

@Andras-Simon
Copy link
Author

@dutiyesh I like it! Lets go with --no-cross-browser

… --no-cross-browser

In more detail:
- if the  crossBrowser env variable is true, browser-polyfill.js will be copied to the build folder. In order to get the env variable from webpack the exported common config object is converted to a function. All other webpack configs that use the common config have to explicitly call this function (with the env param) to get the common config object.

- also, .html files now have a  <%= polyfill => template, which gets replaced on build time. If crossBrowser is true, the browser-polyfill.js file gets included in the html file. Otherwise an empty string will replace the <%= polyfill => template
If the webextenstion support is not enabled, we need to copy the template files with .nocrosbbrowser.js ending.

After that, these files are renamed and the .nocrosbbrowser extension is removed.
… update the original .js files to use the WebExtensions standard
@Andras-Simon
Copy link
Author

@dutiyesh I think the PR is ready, I will test the generated outputs tomorrow.
In the meantime, please have a look at it

Copy link
Owner

@dutiyesh dutiyesh left a comment

Choose a reason for hiding this comment

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

@Andras-Simon Great work!
I have reviewed your pull request with comments, let me know if you have any questions.

And I came across few issues while I was testing:

  • When I tried to create an extension with no-cross-browser command, webpack.config.js file is not getting copied in project's config folder. (Probably because of its Regex)
  • For Popup extension in Firefox, I got this error in console - 'The storage API will not work with a temporary addon ID. Please add an explicit addon ID to your manifest.' (In Popup template, we are using storage API for storing Counter value.)

config/webpack.common.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
templates/devtools/src/background.js Outdated Show resolved Hide resolved
config/webpack.common.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
templates/devtools/src/panel.js Outdated Show resolved Hide resolved
@dutiyesh dutiyesh added the enhancement New feature or request label Nov 2, 2019
@martinmanzo
Copy link

Hi! Any news on this? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants